Implement reentrance detection for std::sync::Once

[nbdd0121]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[ecstatic-morse]

Spinning unconditionally is risky on operating systems with pre-emptive schedulers even if the critical section is short. Admittedly Once only needs to lock once during its lifetime, so the risk is somewhat mitigated in this case. Is it possible to implement reentrance detection while remaining lock-free?

[nbdd0121]

Spinning unconditionally is risky on operating systems with pre-emptive schedulers even if the critical section is short. Admittedly Once only needs to lock once during its lifetime, so the risk is somewhat mitigated in this case. Is it possible to implement reentrance detection while remaining lock-free?

I would like it to remain lock-free as well but I don't think it's possible. The main hurdle is that the ThreadId must be stored somewhere.

• We definitely not want it to be kept with Once, because that's at least a usize penalty for all Once variables, despite it's only used during initialization.
• It cannot be a global static because a thread is allowed to recursively initialize difference Onces.

Therefore it must be kept on the stack of the initializing thread (or on the heap, but the initializing thread would own it anyway, so lifetime is the same). So the state_and_queue would need to point somewhere on this thread's stack (in my PR it is called WaiterQueue).

Now we need to avoid WaiterQueue from being freed while others might reference it. If we want to remain lock-free, it means that a thread would need to atomically read the thread ID, compare it, and insert to the linked list if there's no reentrancy. Such thing requires reading at least 2 usizes atomically, which is not possible. The only sensible approach would therefore to use a lock.

I choose a spin-lock because it happens to be encodable in the usize bits, and I think the risk is acceptable because the critical region is short and this is the cold path (as I mentioned in the commit message).

An alternative way is to use a global mutex to protect it.

BTW, some background knowledge about how most C++ runtime implements lazily initialized function-local static variables: the "once guard" is only required to hold the state. A global reentrant mutex mandates that only one thread may run initializers at one moment (even for different variables). This approach is very simple and probably requires no unsafe code to implement in Rust. It disallows multiple threads from initializing different lazy variables but it can thus detect all possible deadlocks.

[ecstatic-morse]

I would like it to remain lock-free as well but I don't think it's possible. The main hurdle is that the ThreadId must be stored somewhere.

I also don't have a good solution for this.

Personally, I would not choose to sacrifice lock-freedom, especially when the new lock is a spinlock, to be able to detect reentrant call_once invocations. OTOH, it seems like a good idea to detect potential misuse in the standard library, even if it may plausibly impact performance, since the average user probably cares more about finding bugs. Those that do want the lock-free implementation can always switch to a crate.

[nbdd0121]

TBH I wouldn't be too worried about performance of having a lock:

• As mentioned, it is on a cold path and executed at most once per thread for each Once variable.
• The only two places where this lock is acquired is wait and WaiterQueueGuard::Drop: for the first case it'll park later, which is implemented as condvar wait, so it needs to hold a lock anyway; for the second case, if it races with other threads, it means there are other threads sleeping, so it need to unpark them, which will require a mutex to be acquired as well!

So the only thing to be worried is the spinlock. It can always be switched to a global mutex if that's the consensus.

[ecstatic-morse]

For passers-by, here's a blog post that explains the problems with user-space spin-locks. That one is Rust-specific, but many other sources come to the same conclusion. Of course, triggering the pathological behavior is more difficult in this case since it can only happen during initialization.

parking_lot::Mutex would be perfect here, since it is only a single byte. Unfortunately we can't use it in the standard library yet.

[Diggsey]

There's another strategy which does not require any additional locking:

• Store a separate "chain" in a scoped thread-local variable.
• Each link in the thread-local chain stores a pointer to a Once value.
• Before executing the initialization function, push a pointer to the current Once onto the thread-local chain.
• Before parking the current thread, iterate the chain to detect if the current Once value is already present.

Effectively we are walking up the call-stack to check if the same stack frame already exists further up. This only affects the cold path.

[nbdd0121]

There's another strategy which does not require any additional locking:

• Store a separate "chain" in a scoped thread-local variable.
• Each link in the thread-local chain stores a pointer to a Once value.
• Before executing the initialization function, push a pointer to the current Once onto the thread-local chain.
• Before parking the current thread, iterate the chain to detect if the current Once value is already present.

Effectively we are walking up the call-stack to check if the same stack frame already exists further up. This only affects the cold path.

Indeed a very interesting approach! The one caveat is that if we have a chain of call_once then the performance is rather than . Checking the ThreadId, on the other hand, is constant, so it wouldn't make the complexity worse.

Given that parking/unparking would need to take locks anyway I think we could tolerate usage of a lock.

[nbdd0121]

cc @matklad, because the implementation is relevant to OnceCell as well.

[Dylan-DPC-zz]

r? @dtolnay

[dtolnay]

I am not immediately sold on this change, but let me check if others feel we should do this.

@rfcbot poll libs Make std::sync::Once panic instead of deadlock on re-entrant initialization at the expense of complicating the implementation

use std::sync::Once;

static ONCE: Once = Once::new();

fn main() {
    ONCE.call_once(|| ONCE.call_once(|| {}));
}

thread 'main' panicked at 'Once instance cannot be recursively initialized', src/libstd/sync/once.rs:417:39

[rfcbot]

Team member @dtolnay has asked teams: T-libs, for consensus on:

Make std::sync::Once panic instead of deadlock on re-entrant initialization at the expense of complicating the implementation

• @Amanieu
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

[withoutboats]

I think its a net good to turn programmer errors from deadlocks into panics, but I'd want to see concretely the other side of this trade off. It seems like the thread hasn't landed on a consensus for the best implementation. Once its clear what the trade off is I think we should consider if this change is worth it.

[bors]

☔ The latest upstream changes (presumably #74468) made this pull request unmergeable. Please resolve the merge conflicts.

[spastorino]

This is right now tagged as waiting on team libs-impl but I think it's really waiting on T-libs team consensus. Going to add T-libs label but please feel free to correct me if I'm wrong.

[nbdd0121]

Rebased to solve conflicts from #72414. This PR now automatically detects recursive initialization of SyncLazy as well.

#![feature(once_cell)]
use std::lazy::SyncLazy;
static A: SyncLazy<u32> = SyncLazy::new(|| *A);
fn main() {
    println!("{}", *A);
}

gives

thread 'main' panicked at 'Once instance cannot be recursively initialized', src/libstd/sync/once.rs:418:39
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[bors]

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC-zz]

Created a thread on zulip to discuss this further.

[Amanieu]

I think that we definitely don't want to introduce spin-locking in the standard library. Have you considered trying to implement re-entrance detection on parking_lot::Once? We could try reviving the effort to replace the standard library primitives with parking_lot.

[Dylan-DPC-zz]

@nbdd0121 any updates on this?

[Dylan-DPC-zz]

@nbdd0121 thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks