Roll algorithm back to v2.5.3, then add back features to avoid a breaking change #130
Conversation
notgull
changed the title
|
Paging @smol-rs/admins for reviewing this PR. Also paging @mamaicode, @jbr and @beckend as I've re-written some of your commits here. |
|
While I understand why you did it the way you did (to be able to better test/bisect the changes) but it's very hard to review a big bunch of commits like this. Also since you'll likely be squashing, can you already squash all the reverting commits into one (you can add in the description if you only reverted specific commits or it's a hard reset) and then add the new features on top (with each commit describing the change and why it's different now). After that, please do not squash merge when it's time to merge, to maintain the history. I'm sorry for asking you to do all that but I've very limited time these days for smol (as you might have noticed). :( |
I'm man enough to know when I've been wrong. The intrusive-list-based list implementation is riddled with bugs, fails MIRI, and overall increases the complexity of this crate. I've invested a lot of time and energy into improving it; effort which I feel has been wasted. So I figure I'll cut my losses and revert. This PR undoes every commit made between now and the release of event-listener version 2.5.3. The next commits will be replaying newer features in order to avoid a breaking change. Signed-off-by: John Nunley <dev@notgullnet>
@taiki-e's commits since v2.5.3 aren't the problem, as they only change CI/framing details. This commit brings back all work by @taiki-e. Co-authored-by: John Nunley <[email protected]> Signed-off-by: John Nunley <[email protected]>
This commit adds back the <T> parameter on Event and EventListener. So far nothing is done with the generic parameter. However it will eventually be used to bring back tagged events. Signed-off-by: John Nunley <[email protected]>
This commit re-adds the "notify.rs" file pretty much verbatim. This allows for tagged notifications to be used, i.e. convey additional control information along with the notification. The tags are stored in the links with the notification. This also adds some tests for the new functionality. Signed-off-by: John Nunley <[email protected]>
This commit adds back a feature that allows one to use portable-atomic instead of regular atomics. Kind of useless without no-std, but it's here again. Signed-off-by: John Nunley <[email protected]>
The Listener trait was previously used to abstract over heap and stack based event listeners. However at this point we only have heap based event listeners. Still, to avoid a breaking change we need to have this trait. Other minor fixups include: - Make the locking ignore poisoning. - Reduce formatting code Signed-off-by: John Nunley <[email protected]>
For now it just creates a listener on the heap. We can rework this later, maybe to include stack listeners. For now it's best to keep it this way, I feel. Signed-off-by: John Nunley <[email protected]>
This way added earlier as a way of telling how many listeners were woken up by a notify() call. It's very simple to implement with the mutex strategy. Signed-off-by: John Nunley <[email protected]>
This also makes sure that MIRI can run here. Signed-off-by: John Nunley <[email protected]>
The reverts are all squashed into 6368046. The rest of the commits are re-adding the functionality.
Done.
Sounds good!
All good, no worries |
notgull
force-pushed
the
notgull/revert
branch
3 times, most recently
from
46d4236 to
aa63e03
Compare
Replay of #31 with minor modifications Signed-off-by: John Nunley <[email protected]>
- Update Cargo.toml and CHANGELOG.md - Add back the smol-rs logo (#63) Signed-off-by: John Nunley <[email protected]>
cc #48 and #59 Signed-off-by: John Nunley <[email protected]>
cc #86, it's harder to get this info on no_std so I've ignored it for now Signed-off-by: John Nunley <[email protected]>
Mirror of #114 Signed-off-by: John Nunley <[email protected]>
notgull
force-pushed
the
notgull/revert
branch
2 times, most recently
from
e9c1f04 to
62ce8fe
Compare
Co-authored-by: Jacob Rothstein <[email protected]> Signed-off-by: John Nunley <[email protected]>
|
@notgull what stops you from merging this? |
I'm waiting for reviews from other |
|
Actually, come to think of it this PR might be a little much. @smol-rs/admins Would it be preferred if I split this up into smaller PRs, starting with an initial rollback followed by adding on more features? |
I can't say for others but what makes this PR hard to review for me, is it's hard to see what really changed in the end. I understand why you did it the way you did, to be able to figure out where the regression(s) happened but now that you know, I think it would be much better if we don't go back in history, but instead add the needed fixes on top of the current main branch (i-e not doing rollbacks, revert of specific commits is of course good). WDYT? |
The issue is that I'm not 100% sure what the current regressions are caused by, and the current advantages of the current implementation are not worth the effort of my part to investigate them. |
|
If you have proven stability by adding all these tests, why not call it good enough of a change and merge it. |
Fair enough but then how do we ensure that the regressions weren't re-added by the "new" commits? For all we know, there could be a timing issue and tests don't catch the issue(s) still present. Finding out the issue and fixing it, would be the only certain way of actually fixing the problems. I would hate for us to have to break API again and again. |
|
Moreover, if there are still regressions/issue, merging this PR as is, would make it super hard to bisect the history to figure out the culprit commits/changes later on. In any case, since I'm not that involved in the project anymore and you're doing the work, I'd leave it to you how you want to proceed. I would request that you consider my advice here. |
I'm using the But, fair enough. I'll do a deep dive into the current code and see if I can figure out what the issue is. |
I appreciate your very constructive attitude to my rather blunt feedback. 👍 |
|
cc #133 |
The notify() function contains two optimizations: - If the `inner` field is not yet initialized (i.e. no listeners have been created yet), it returns `0` from the function early. - If the atomic `notified` field indicates that the current notification would do nothing, it returns `0` early as well. `loom` testing has exposed race conditions in these optimizations that can cause notifications to be missed, leading to deadlocks. This commit removes these optimizations in the hope of preventing these deadlocks. Ideally we can restore them in the future. Closes #138 cc #130 and #133 Signed-off-by: John Nunley <[email protected]>
|
Obsoleted by #139 |
The notify() function contains two optimizations: - If the `inner` field is not yet initialized (i.e. no listeners have been created yet), it returns `0` from the function early. - If the atomic `notified` field indicates that the current notification would do nothing, it returns `0` early as well. `loom` testing has exposed race conditions in these optimizations that can cause notifications to be missed, leading to deadlocks. This commit removes these optimizations in the hope of preventing these deadlocks. Ideally we can restore them in the future. Closes #138 cc #130 and #133 Signed-off-by: John Nunley <[email protected]>
I have to apologize. I know it sounds self-deprecating, but really, I've been over the line on this particular issue. This should hopefully fix it for now.
The new implementation for the event listener, #51, has been plagued with bugs from its inception. Frankly, I've gotten sick of papering over these bugs, and I know that prod customers are almost certainly impacted but haven't reported it.
This PR reverts the implementation of
event-listenerback to version 2.5.3; before I made any of the sweeping architectural changes to the crate. Then I re-add new changes to this crate in a "path of least resistance" fashion. The goal is to change the internal algorithm as little as possible, as we know this algorithm is stable and robust.The only real diversion from this path is my adding of a
no_stdimplementation. I've added ano_stdimplementation based on an atomic linked list built on an amortized atomically expanded vector. Since it doesn't useconcurrent-queueit fixes #109.Let me run a "crater test" on the
smolecosystem before this is merged. Expect a "post mortem" at some point in the near future.