Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix notifier list #2

Merged
merged 2 commits into from Oct 29, 2022
Merged

Fix notifier list #2

merged 2 commits into from Oct 29, 2022

Conversation

sbarral
Copy link
Member

@sbarral sbarral commented Oct 27, 2022

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 the WaitUntil future an unsafe leaky abstraction and made it necessary to mark pretty much everything around it as unsafe, 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 and event.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.

src/lib.rs Show resolved Hide resolved
@sbarral sbarral merged commit 2e5b4a5 into main Oct 29, 2022
@sbarral sbarral deleted the fix-notifier-list branch October 29, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants