Skip to content

Commit

Permalink
Auto merge of rust-lang#114591 - joboet:thread_parking_ordering_fix, …
Browse files Browse the repository at this point in the history
…r=thomcc

Synchronize with all calls to `unpark` in id-based thread parker

[The documentation for `thread::park`](https://doc.rust-lang.org/nightly/std/thread/fn.park.html#memory-ordering) guarantees that "park synchronizes-with all prior unpark operations". In the id-based thread parking implementation, this is not implemented correctly, as the state variable is reset with a simple store, so there will not be a *synchronizes-with* edge if an `unpark` happens just before the reset. This PR corrects this, replacing the load-check-reset sequence with a single `compare_exchange`.
  • Loading branch information
bors committed Aug 18, 2023
2 parents ee5cb9e + fd9fcc5 commit d06ca0f
Showing 1 changed file with 6 additions and 11 deletions.
17 changes: 6 additions & 11 deletions library/std/src/sys_common/thread_parking/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,18 +56,14 @@ impl Parker {
self.init_tid();

// Changes NOTIFIED to EMPTY and EMPTY to PARKED.
let mut state = self.state.fetch_sub(1, Acquire).wrapping_sub(1);
if state == PARKED {
let state = self.state.fetch_sub(1, Acquire);
if state == EMPTY {
// Loop to guard against spurious wakeups.
while state == PARKED {
// The state must be reset with acquire ordering to ensure that all
// calls to `unpark` synchronize with this thread.
while self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Relaxed).is_err() {
park(self.state.as_ptr().addr());
state = self.state.load(Acquire);
}

// Since the state change has already been observed with acquire
// ordering, the state can be reset with a relaxed store instead
// of a swap.
self.state.store(EMPTY, Relaxed);
}
}

Expand All @@ -78,8 +74,7 @@ impl Parker {
if state == PARKED {
park_timeout(dur, self.state.as_ptr().addr());
// Swap to ensure that we observe all state changes with acquire
// ordering, even if the state has been changed after the timeout
// occurred.
// ordering.
self.state.swap(EMPTY, Acquire);
}
}
Expand Down

0 comments on commit d06ca0f

Please sign in to comment.