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

Drop wakers after unlocking the mutex in Notify #5471

Merged
merged 2 commits into from
Feb 19, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions tokio/src/sync/notify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -917,10 +917,17 @@ impl Notified<'_> {
}
}

let mut old_waker = None;
if waker.is_some() {
// Safety: called while locked.
//
// This code makes the following assignment, but it
// stores the old waker in `old_waker` so that we can
// drop it after releasing the mutex.
//
// (*waiter.get()).waker = waker;
unsafe {
(*waiter.get()).waker = waker;
old_waker = std::mem::replace(&mut (*waiter.get()).waker, waker);
Copy link
Member

Choose a reason for hiding this comment

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

old_waker is always None here, as this is the first place we store a waker and from here we always transition to Waiting with no return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am reluctant to rely on assumptions like "waker is always None here" or "waker is always Some here", and prefer to program defensively. We've run into trouble with those in the past due to this type of assumption being broken by future changes to the code.

Anyway, I added a comment to note this.

}
}

Expand All @@ -931,6 +938,9 @@ impl Notified<'_> {

*state = Waiting;

drop(waiters);
drop(old_waker);

return Poll::Pending;
}
Waiting => {
Expand All @@ -945,12 +955,13 @@ impl Notified<'_> {

// Safety: called while locked
let w = unsafe { &mut *waiter.get() };
let mut old_waker = None;

if w.notified.is_some() {
// Our waker has been notified and our waiter is already removed from
// the list. Reset the notification and convert to `Done`.
old_waker = std::mem::take(&mut w.waker);
w.notified = None;
w.waker = None;
*state = Done;
} else if get_num_notify_waiters_calls(curr) != *notify_waiters_calls {
// Before we add a waiter to the list we check if these numbers are
Expand All @@ -960,7 +971,7 @@ impl Notified<'_> {
// We can treat the waiter as notified and remove it from the list, as
// it would have been notified in the `notify_waiters` call anyways.

w.waker = None;
old_waker = std::mem::take(&mut w.waker);

// Safety: we hold the lock, so we have an exclusive access to the list.
// The list is used in `notify_waiters`, so it must be guarded.
Expand All @@ -975,10 +986,14 @@ impl Notified<'_> {
None => true,
};
if should_update {
w.waker = Some(waker.clone());
old_waker = std::mem::replace(&mut w.waker, Some(waker.clone()));
}
}

// Drop the old waker after releasing the lock.
drop(waiters);
drop(old_waker);

return Poll::Pending;
}

Expand All @@ -988,6 +1003,9 @@ impl Notified<'_> {
// is helpful to visualize the scope of the critical
// section.
drop(waiters);

// Drop the old waker after releasing the lock.
drop(old_waker);
}
Done => {
return Poll::Ready(());
Expand Down