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

sync: fix notification permit getting dropped on receiver drop #3652

Merged
merged 1 commit into from Mar 30, 2021

Conversation

simonlindholm
Copy link
Contributor

Noticed this small bug when reading through notify.rs.

Motivation

The Drop implementation for sync::notify::Notified has a case where it sets the notification state to EMPTY, regardless of previous state. This is motivated by a comment claiming that this is correct in all cases except when the previous state is NOTIFIED, in which case the state will be changed back to NOTIFIED further down in notify_locked. But with the introduction of notify_waiters this is no longer always true, because that call is filtered by the notification type being NotificationType::OneWaiter. Indeed the following can happen:

n = Notify::new()
a = n.notified()
a.poll()            // adds a to waiters list, sets state to WAITING
n.notify_waiters()  // removes a from waiters list, sets state to EMPTY
n.notify_one()      // sets state to NOTIFIED
drop(a)             // sets state to EMPTY
n.notified().await  // waits forever

Solution

Condition the write on state being WAITING. (I'm guessing the previous code is a remnant from when state wasn't already previously being read higher up in the function?)

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Mar 27, 2021
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems good to me.

@Darksonn Darksonn merged commit f7c181c into tokio-rs:master Mar 30, 2021
@Darksonn Darksonn mentioned this pull request Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants