Document memory orderings of thread::{park, unpark}

[ibraheemdev]

Document thread::park/unpark as having acquire/release synchronization. Without that guarantee, even the example in the documentation can deadlock:

let flag = Arc::new(AtomicBool::new(false));

let t2 = thread::spawn(move || {
    while !flag.load(Ordering::Acquire) {
        thread::park();
    }
});

flag.store(true, Ordering::Release);
t2.thread().unpark();

// t1: flag.store(true)
// t1: thread.unpark()
// t2: flag.load() == false

// t2 now parks, is immediately unblocked but never
// acquires the flag, and thus spins forever 

Multiple calls to unpark should also maintain a release sequence to make sure operations released by previous unparks are not lost:

let a = Arc::new(AtomicBool::new(false));
let b = Arc::new(AtomicBool::new(false));

let t2 = thread::spawn(move || {
    while !a.load(Ordering::Acquire) || !b.load(Ordering::Acquire) {
        thread::park();
    }
});

thread::spawn(move || {
    a.store(true, Ordering::Release);
    t2.thread().unpark();
});

b.store(true, Ordering::Release);
t2.thread().unpark();

// t1: a.store(true)
// t1: t2.unpark()
// t3: b.store(true)
// t3: t2.unpark()

// t2 now parks, is immediately unblocked but never
// acquires the store of `a`, only the store of `b` which
// was released by the most recent unpark, and thus spins forever 

This is of course a contrived example, but is reasonable to rely upon in real code.

Note that all implementations of park/unpark already comply with the rules, it's just undocumented.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[ibraheemdev]

@rustbot label +T-libs-api -T-libs

[faern]

I came here to file an issue just like this one. The way I interpret the docs and the example, they are currently conflicting. Either the API should promise synchronization, like the PR currently proposes, or the example should have flag.store(true, Ordering::Release); changed into flag.swap(true, Ordering::AcqRel); in order to ensure the unparking happens after the flag has been set.

EDIT: With AcqRel, the store needs to be a swap

[faern]

Any updates on this? It would be great to know if one can rely on park/unpark performing synchronization or not.

I'm guessing that the existence of the current example, which implies synchronization, has lead to a bunch of code in the wild assuming synchronization. So maybe it's too dangerous to make the implementation not synchronize now?

[Riolku]

Any updates on this? It would be great to know if one can rely on park/unpark performing synchronization or not.

I'm guessing that the existence of the current example, which implies synchronization, has lead to a bunch of code in the wild assuming synchronization. So maybe it's too dangerous to make the implementation not synchronize now?

I do agree that it is likely that many code bases assume synchronization, but can we find concrete examples? The best example would be one in std I feel, but any other library is fine too.

[joshtriplett]

r? @m-ou-se

[geeklint]

What's the status of this PR? I just came across the docs myself and was confused. Specifically the sentence

Notice that being unblocked does not imply any synchronization with someone that unparked this thread, it could also be spurious.

seems to have two possible interpretations, option A:

The park call returning does not imply another thread has called unpark, because it could have been spurious.

or option B:

There is no synchronization between park and unpark. By the way, park can return spuriously.

Option A seems to be be the required interpretation for the functions to be useful, but on my first reading of the docs option B was how I interpreted that sentence. This ambiguous phrasing is still in the updated documentation, but the additional information on memory orderings seems to clarify the intent better.

[ibraheemdev]

@geeklint I agree and updated the wording to be a little clearer. This PR is still waiting for a review.

[m-ou-se]

Sorry for the delay. Moving this to the top of my to do list for tomorrow.

[m-ou-se]

About the guarantee: I completely agree we should guarantee this. Not because of memory safety (since waking up can always be spurious), but (as you mention) to prevent deadlocks. We should do a libs-api FCP for that, but I expect that to be uncontroversial.

About this specific patch:

The explanation looks correct. Perhaps we could make it even clearer that, because of the spurious wake ups, this is not something useful for memory safety, but only to prevent deadlocks.

To make the guarantee clearer, let's update the example to use Relaxed ordering instead:

/// ```
/// use std::thread;
/// use std::sync::atomic::{Ordering, AtomicBool};
/// use std::time::Duration;
///
/// let flag = AtomicBool::new(false);
///
/// thread::scope(|s| {
///     let thread = s.spawn(|| {
///         // We want to wait until the flag is set. We *could* just spin, but using
///         // park/unpark is more efficient.
///         while !flag.load(Ordering::Relaxed) {
///             println!("Parking thread.");
///             thread::park();
///             // We could get here spuriously, i.e., way before the 10ms below are over.
///             // But that is no problem, because of the loop.
///             println!("Thread unparked.");
///         }
///         println!("Flag received.");
///     });
///
///     // Let some time pass.
///     thread::sleep(Duration::from_millis(10));
///
///     // Set the flag and unpark the thread.
///     flag.store(true, Ordering::Relaxed);
///     println!("Flag set. Unparking the thread.");
///     thread.thread().unpark();
/// });
/// ```

[RalfJung]

That's not how memory models work, they are not defined in terms of reordering, they are defined in terms of synchronization and event visibility.

I guess the point is that if park doesn't establish a synchronization edge with the prior unpark, then the 2nd time around the loop it would still be legal for t2 not to see the store(true), so it parks again. And now it doesn't wake up any more since there is no 2nd unpark?
In contrast, if the park when it returns has a synchronization edge, then it has acquired all the events that occurred in t1 prior to unpark, including the store, and we are good.

This becomes more complicated if there are other unpark of the same thread elsewhere in the program though. But even then, either the other unparks go on indefinitely and then (by memory fairness) we will see the store eventually, or the other unparks stop eventually and then we will "use up" the unpark from t1 and synchronize accordingly.

[ibraheemdev]

@RalfJung I fixed the trace description. Yes, the issue is that with no synchronization between park and unpark, a wakeup from park is not guaranteed to see the memory modified by the thread that called unpark, and so it may park again and never be woken up.

The other issue is that even if there is acquire/release synchronization, acquiring only the memory modified by the latest thread to call unpark is not enough for cases like the second example. We also need to document the release sequence that is maintained by calls to unpark across threads.

This synchronization is generally assumed to exist, without it park/unpark wouldn't be very useful, but it hasn't been explicitly documented.

[RalfJung]

The other issue is that even if there is acquire/release synchronization, acquiring only the memory modified by the latest thread to call unpark is not enough for cases like the second example. We also need to document the release sequence that is maintained by calls to unpark across threads.

Right, in my previous post I wrongly assumed that calling unpark() multiple times would actually lead to N following park() waking up, but that's not how this works -- there is at most one "unparking token" available for the next park() to return immediately. However by synchronizing-with all prior unpark() calls, the situation is handled properly: eventually t1 will unpark, then maybe there'll be a munch more unpark, then t2 will park and return and thereby synchronize-with t1 so the next load will see t1's store.

[ibraheemdev]

@m-ou-se I relaxed the orderings in the example, this should be ready to merge.

[m-ou-se]

@bors r+ rollup

[bors]

📌 Commit 3acb1d2 has been approved by m-ou-se

It is now in the queue for this repository.

[faern]

I think the new documentation contradicts itself. Now when we guarantee memory synchronization, empty implementations of park and unpark are no longer valid implementations.

Calls to park synchronize-with calls to unpark, meaning that memory operations performed before a call to unpark are made visible to the thread that consumes the token and returns from park. ...

Is contradicted by:

... a valid, but inefficient, implementation could have park and unpark return immediately without doing anything ...

If park and unpark indeed was implemented as empty functions, they would not synchronize at all. They have to at least include a memory barrier to uphold that, right?

Minimal valid implementation:

pub fn park() {
    fence(Ordering::Acquire);
}

impl Thread {
    pub fn unpark(&self) {
        fence(Ordering::Release);
    }
}

[m-ou-se]

That was discussed: see #99587 (comment):

It would be correct, because in that case park() never consumes any tokens. With such a trivial implementation, all wake-ups are spurious.

[ibraheemdev]

@faeen the new guarantees are only there to prevent deadlocks. You already need external synchronization (i.e a separate flag) to properly synchronize with other objects, due to possible calls to unpark from other threads.