Skip to content

Document guarantees of poisoning #144185

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions library/std/src/sync/once_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use crate::sync::Once;
/// A `OnceLock` can be thought of as a safe abstraction over uninitialized data that becomes
/// initialized once written.
///
/// Unlike [`Mutex`](crate::sync::Mutex), `OnceLock` is never poisoned on panic.
///
/// [`OnceCell`]: crate::cell::OnceCell
/// [`LazyLock<T, F>`]: crate::sync::LazyLock
/// [`LazyLock::new(|| ...)`]: crate::sync::LazyLock::new
Expand Down
24 changes: 13 additions & 11 deletions library/std/src/sync/poison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@
//!
//! # Poisoning
//!
//! All synchronization objects in this module implement a strategy called "poisoning"
//! where if a thread panics while holding the exclusive access granted by the primitive,
//! the state of the primitive is set to "poisoned".
//! This information is then propagated to all other threads
//! All synchronization objects in this module implement a strategy called
//! "poisoning" where a primitive becomes poisoned if it recognizes that some
//! thread has panicked while holding the exclusive access granted by the
//! primitive. This information is then propagated to all other threads
//! to signify that the data protected by this primitive is likely tainted
//! (some invariant is not being upheld).
//!
//! The specifics of how this "poisoned" state affects other threads
//! depend on the primitive. See [#Overview] below.
//! The specifics of how this "poisoned" state affects other threads and whether
//! the panics are recognized reliably or on a best-effort basis depend on the
//! primitive. See [#Overview] below.
//!
//! For the alternative implementations that do not employ poisoning,
//! see `std::sync::nonpoisoning`.
Expand All @@ -34,14 +35,15 @@
//! - [`Mutex`]: Mutual Exclusion mechanism, which ensures that at
//! most one thread at a time is able to access some data.
//!
//! [`Mutex::lock()`] returns a [`LockResult`],
//! providing a way to deal with the poisoned state.
//! See [`Mutex`'s documentation](Mutex#poisoning) for more.
//! Panicking while holding the lock typically poisons the mutex, but it is
//! not guaranteed to detect this condition in all circumstances.
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to clarify that poisoning is only unreliable when you have multiple pending panics or foreign exceptions. In particular it works fine in the "normal" case of a single panic.

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 wasn't sure if we wanted to commit to that guarantee. Consider the following situation:

  • A panic is thrown.
  • A drop guard locks a mutex and stores the guard into a cell. The guard records panicking: true.
  • The drop guard exists and the panic is caught and dropped.
  • Another panic is thrown.
  • Another drop guard loads the guard from the cell and drops it. Since the guard has panicking: true and we're currently panicking as well, the mutex is not poisoned.

Yet there's never two active panics at the same time. Only one panic has been thrown while the mutex was locked -- the other panic was only caught.

That's a lot of detail I don't know if I can formalize in a few words in the docs without making a mistake and guaranteeing something we can't provide.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with your suggested docs is that the semantics are too vague to be of any use. Essentially we're saying that if a panic occurs while a MutexGuard is live then the lock may or may not be poisoned. While it's fine to not make hard guarantees, it would be nice to have a reassuring sentence along the lines of "if you're not doing fancy things like multiple live panics or preserving a MutexGuard across a catch_unwind then it will usually work as you expect".

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively give specific examples of cases where poisoning doesn't work properly, with a note that this list isn't exhaustive. This will at least reassure people that poisoning probably works if they're not doing those things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I've added a list. Does this look good?

Copy link
Member

Choose a reason for hiding this comment

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

Perfect! I'd just add another bullet point about foreign exceptions and this should be good to go.

Copy link
Contributor Author

@purplesyringa purplesyringa Jul 25, 2025

Choose a reason for hiding this comment

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

Totally forgot about that, thanks for the reminder. I added a note.

Copy link
Contributor Author

@purplesyringa purplesyringa Jul 29, 2025

Choose a reason for hiding this comment

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

@Amanieu sorry for pinging -- could I get an r+?

//! [`Mutex::lock()`] returns a [`LockResult`], providing a way to deal with
//! the poisoned state. See [`Mutex`'s documentation](Mutex#poisoning) for more.
//!
//! - [`Once`]: A thread-safe way to run a piece of code only once.
//! Mostly useful for implementing one-time global initialization.
//!
//! [`Once`] is poisoned if the piece of code passed to
//! [`Once`] is reliably poisoned if the piece of code passed to
//! [`Once::call_once()`] or [`Once::call_once_force()`] panics.
//! When in poisoned state, subsequent calls to [`Once::call_once()`] will panic too.
//! [`Once::call_once_force()`] can be used to clear the poisoned state.
Expand All @@ -51,7 +53,7 @@
//! writer at a time. In some cases, this can be more efficient than
//! a mutex.
//!
//! This implementation, like [`Mutex`], will become poisoned on a panic.
//! This implementation, like [`Mutex`], usually becomes poisoned on a panic.
//! Note, however, that an `RwLock` may only be poisoned if a panic occurs
//! while it is locked exclusively (write mode). If a panic occurs in any reader,
//! then the lock will not be poisoned.
Expand Down
52 changes: 43 additions & 9 deletions library/std/src/sync/poison/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,54 @@ use crate::sys::sync as sys;
/// # Poisoning
///
/// The mutexes in this module implement a strategy called "poisoning" where a
/// mutex is considered poisoned whenever a thread panics while holding the
/// mutex. Once a mutex is poisoned, all other threads are unable to access the
/// data by default as it is likely tainted (some invariant is not being
/// upheld).
/// mutex becomes poisoned if it recognizes that the thread holding it has
/// panicked.
///
/// For a mutex, this means that the [`lock`] and [`try_lock`] methods return a
/// Once a mutex is poisoned, all other threads are unable to access the data by
/// default as it is likely tainted (some invariant is not being upheld). For a
/// mutex, this means that the [`lock`] and [`try_lock`] methods return a
/// [`Result`] which indicates whether a mutex has been poisoned or not. Most
/// usage of a mutex will simply [`unwrap()`] these results, propagating panics
/// among threads to ensure that a possibly invalid invariant is not witnessed.
///
/// A poisoned mutex, however, does not prevent all access to the underlying
/// data. The [`PoisonError`] type has an [`into_inner`] method which will return
/// the guard that would have otherwise been returned on a successful lock. This
/// allows access to the data, despite the lock being poisoned.
/// Poisoning is only advisory: the [`PoisonError`] type has an [`into_inner`]
/// method which will return the guard that would have otherwise been returned
/// on a successful lock. This allows access to the data, despite the lock being
/// poisoned.
///
/// In addition, the panic detection is not ideal, so even unpoisoned mutexes
/// need to be handled with care, since certain panics may have been skipped.
/// Therefore, `unsafe` code cannot rely on poisoning for soundness. Here's an
/// example of **incorrect** use of poisoning:
///
/// ```rust
/// use std::sync::Mutex;
///
/// struct MutexBox<T> {
/// data: Mutex<*mut T>,
/// }
///
/// impl<T> MutexBox<T> {
/// pub fn new(value: T) -> Self {
/// Self {
/// data: Mutex::new(Box::into_raw(Box::new(value))),
/// }
/// }
///
/// pub fn replace_with(&self, f: impl FnOnce(T) -> T) {
/// let ptr = self.data.lock().expect("poisoned");
/// // While `f` is running, the data is moved out of `*ptr`. If `f`
/// // panics, `*ptr` keeps pointing at a dropped value. The intention
/// // is that this will poison the mutex, so the following calls to
/// // `replace_with` will panic without reading `*ptr`. But since
/// // poisoning is not guaranteed to occur, this can lead to
/// // use-after-free.
/// unsafe {
/// (*ptr).write(f((*ptr).read()));
/// }
/// }
/// }
/// ```
///
/// [`new`]: Self::new
/// [`lock`]: Self::lock
Expand Down
6 changes: 5 additions & 1 deletion library/std/src/sync/poison/once.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ impl Once {
/// it will *poison* this [`Once`] instance, causing all future invocations of
/// `call_once` to also panic.
///
/// This is similar to [poisoning with mutexes][poison].
/// This is similar to [poisoning with mutexes][poison], but this mechanism
/// is guaranteed to never skip panics within `f`.
///
/// [poison]: struct.Mutex.html#poisoning
#[inline]
Expand Down Expand Up @@ -293,6 +294,9 @@ impl Once {

/// Blocks the current thread until initialization has completed, ignoring
/// poisoning.
///
/// If this [`Once`] has been poisoned, this function blocks until it
/// becomes completed, unlike [`Once::wait()`], which panics in this case.
#[stable(feature = "once_wait", since = "1.86.0")]
pub fn wait_force(&self) {
if !self.inner.is_completed() {
Expand Down
8 changes: 4 additions & 4 deletions library/std/src/sync/poison/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ use crate::sys::sync as sys;
///
/// # Poisoning
///
/// An `RwLock`, like [`Mutex`], will become poisoned on a panic. Note, however,
/// that an `RwLock` may only be poisoned if a panic occurs while it is locked
/// exclusively (write mode). If a panic occurs in any reader, then the lock
/// will not be poisoned.
/// An `RwLock`, like [`Mutex`], will usually become poisoned on a panic. Note,
/// however, that an `RwLock` may only be poisoned if a panic occurs while it is
/// locked exclusively (write mode). If a panic occurs in any reader, then the
/// lock will not be poisoned.
Copy link
Member

Choose a reason for hiding this comment

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

This should point to the module-level documentation to explain the "usually".

Copy link
Contributor Author

@purplesyringa purplesyringa Jul 25, 2025

Choose a reason for hiding this comment

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

My PR describes the pitfalls in Mutex docs, rather than module-level docs, since they are specific to Mutex/RwLock and not other poisoning locks. I'll make "usually" point to the "Poisoning" section in Mutex docs instead, if that's okay with you.

///
/// # Examples
///
Expand Down
Loading