Skip to content

Fix thread park/unpark synchronization #54174

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Fix thread park/unpark synchronization #54174

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
204d960
Fix `thread` `park`/`unpark` synchronization
Sep 11, 2018
f8a78bd
Add comments and assertion to `park`/`unpark`
Sep 14, 2018
a3b8705
Expand synchronization comments in `park`/`unpark`
Sep 18, 2018
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
29 changes: 11 additions & 18 deletions src/libstd/thread/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,7 @@ pub fn park() {
match thread.inner.state.compare_exchange(EMPTY, PARKED, SeqCst, SeqCst) {
Ok(_) => {}
Err(NOTIFIED) => {
thread.inner.state.store(EMPTY, SeqCst);
thread.inner.state.swap(EMPTY, SeqCst);
Copy link
Contributor

@willmo willmo Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was wondering if this should be a .compare_exchange().unwrap(), just to be more explicit and make some use of the read value (thus possibly reducing the chance of somebody changing this back to .store() in the future). This shouldn't be a common case so I don't imagine the performance impact would be very noticeable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, I will add a comment and an assertion.

return;
} // should consume this notification, so prohibit spurious wakeups in next park.
Err(_) => panic!("inconsistent park state"),
Expand Down Expand Up @@ -889,7 +889,7 @@ pub fn park_timeout(dur: Duration) {
match thread.inner.state.compare_exchange(EMPTY, PARKED, SeqCst, SeqCst) {
Ok(_) => {}
Err(NOTIFIED) => {
thread.inner.state.store(EMPTY, SeqCst);
thread.inner.state.swap(EMPTY, SeqCst);
Copy link

Choose a reason for hiding this comment

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

Maybe add a comment that why swap rather than store? It's possible that the state goes to NOTIFIED just before this line and we need to acknowledge all notifications with an Acquire load (at the very least).

return;
} // should consume this notification, so prohibit spurious wakeups in next park.
Err(_) => panic!("inconsistent park_timeout state"),
Expand Down Expand Up @@ -1058,23 +1058,16 @@ impl Thread {
/// [park]: fn.park.html
#[stable(feature = "rust1", since = "1.0.0")]
pub fn unpark(&self) {
loop {
match self.inner.state.compare_exchange(EMPTY, NOTIFIED, SeqCst, SeqCst) {
Ok(_) => return, // no one was waiting
Err(NOTIFIED) => return, // already unparked
Err(PARKED) => {} // gotta go wake someone up
_ => panic!("inconsistent state in unpark"),
}

// Coordinate wakeup through the mutex and a condvar notification
let _lock = self.inner.lock.lock().unwrap();
match self.inner.state.compare_exchange(PARKED, NOTIFIED, SeqCst, SeqCst) {
Ok(_) => return self.inner.cvar.notify_one(),
Err(NOTIFIED) => return, // a different thread unparked
Err(EMPTY) => {} // parked thread went away, try again
_ => panic!("inconsistent state in unpark"),
}
match self.inner.state.swap(NOTIFIED, SeqCst) {
Copy link

Choose a reason for hiding this comment

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

It's worth adding a comment saying that even if the thread is already notified, we still want to do a write with Release semantics (at the very least).

EMPTY => return, // no one was waiting
NOTIFIED => return, // already unparked
PARKED => {} // gotta go wake someone up
_ => panic!("inconsistent state in unpark"),
}

// Coordinate wakeup through the mutex and a condvar notification
let _lock = self.inner.lock.lock().unwrap();
self.inner.cvar.notify_one()
}

/// Gets the thread's unique identifier.
Expand Down