Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So this is my 3rd rewrite of a fix for issue #1. Hopefully 3rd time is the charm...
What I tried
First attempt
I initially tried the approach which I suggested at the bottom of the issue: box the notifiers and make
Sender
ensure that the notifier is no longer in the list before it is deallocated. While I believe this was a valid approach, it made theWaitUntil
future an unsafe leaky abstraction and made it necessary to mark pretty much everything around it asunsafe
, making the code ugly and brittle.Second attempt
I then decided to take advantage of structural pinning in the
WaitUntil
future and store the notifier inline, which is I guess the approach hinted at by @Darksonn. It looked great since no allocation was needed, and I even figured I could still cache the waker with an approach similar to what I use in this PR. The problem is the cognitive load necessary to make this sound and keep it sound as the code evolves. A good reading on this topic is Sabrina Jewson's pinned-aliasable crate and her ultimate tour-de-force, the pin-list crate.Third attempt
After this sobering dive into pin-based intrusive lists, I decided to keep things simple and use the tried-and-true approach to intrusive list, using only boxed notifiers but this time passing them by value so that the
WaitUntil
future owns the boxed notifier until it is removed from the intrusive list (or forever if forgotten). On future completion, the boxed notifier returns by value to the sender to be cached with its waker, so the notifier is only ever boxed once unless the previous send was not polled to completion.What now?
I will leave this PR sit here a couple of days in case anyone was willing to review it (many thanks in advance if anyone does!), and to give myself a little more time to think about it.
The only interesting files to review are
lib.rs
andevent.rs
(second commit). The others changes only relates to adding new tests that would catch this issue. They made the diff bigger than I'd like because I had to segregate tests that can legitimately leak memory and must run with-Zmiri-ignore-leaks
.What after
After this is merged, I will proceed with a small commit adding inlining hints, because for some reason this fix throws off rustc a little and impacts performance even though it shouldn't, but I have identified the culprit and this can be easily repaired.