Skip to content

Fix LazyLock poison panic message #145307

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 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
40 changes: 29 additions & 11 deletions library/std/src/sync/lazy_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ use super::poison::once::ExclusiveState;
use crate::cell::UnsafeCell;
use crate::mem::ManuallyDrop;
use crate::ops::{Deref, DerefMut};
use crate::panic::{RefUnwindSafe, UnwindSafe};
use crate::panic::{self, AssertUnwindSafe, RefUnwindSafe, UnwindSafe};
use crate::sync::Once;
use crate::sys::sync::once::ONCE_POISON_PANIC_MSG;
use crate::{fmt, ptr};

// We use the state of a Once as discriminant value. Upon creation, the state is
Expand Down Expand Up @@ -244,21 +245,38 @@ impl<T, F: FnOnce() -> T> LazyLock<T, F> {
#[inline]
#[stable(feature = "lazy_cell", since = "1.80.0")]
pub fn force(this: &LazyLock<T, F>) -> &T {
this.once.call_once(|| {
// SAFETY: `call_once` only runs this closure once, ever.
let data = unsafe { &mut *this.data.get() };
let f = unsafe { ManuallyDrop::take(&mut data.f) };
let value = f();
data.value = ManuallyDrop::new(value);
});
let call_once_closure = || {
this.once.call_once(|| {
// SAFETY: `call_once` only runs this closure once, ever.
let data = unsafe { &mut *this.data.get() };
let f = unsafe { ManuallyDrop::take(&mut data.f) };
let value = f();
data.value = ManuallyDrop::new(value);
});
};

// If the internal `Once` is poisoned, it will panic with "Once instance has previously been
// poisoned", but we want to panic with "LazyLock instance ..." instead.
let result = panic::catch_unwind(AssertUnwindSafe(call_once_closure));
Copy link
Member

Choose a reason for hiding this comment

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

catch_unwind can't be used to override the panic message. By the time catch_unwind returns, the panic message has already been printed. In addition catch_unwind would cause an abort in case of a foreign exception.

Maybe you can have an internal method like call_once except with a panic message as argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that does make sense, but at the same time I think it's fine if the user gets 2 error messages?

I'm a bit worried about the footprint of updating the internal call_once method with a panic message as argument since that would require changing all of the underlying implementations as well. But if that is the only acceptable way then I'm happy to do it


if let Err(payload) = result {
// Check if this is the `Once` poison panic.
if let Some(s) = payload.downcast_ref::<String>()
&& s == ONCE_POISON_PANIC_MSG
{
panic_poisoned();
} else {
// This panic came from the closure `f`, so we don't need to change the message.
panic::resume_unwind(payload);
}
}

// SAFETY:
// There are four possible scenarios:
// * the closure was called and initialized `value`.
// * the closure was called and panicked, so this point is never reached.
// * the closure was called and panicked, which we handled above.
// * the closure was not called, but a previous call initialized `value`.
// * the closure was not called because the Once is poisoned, so this point
// is never reached.
// * the closure was not called because the Once is poisoned, which we handled above.
// So `value` has definitely been initialized and will not be modified again.
unsafe { &*(*this.data.get()).value }
}
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/sync/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod condvar;
mod mutex;
mod once;
pub(crate) mod once; // For `ONCE_POISON_PANIC_MSG`.
mod once_box;
mod rwlock;
mod thread_parking;
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/sync/once/futex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ impl Once {
COMPLETE => return,
POISONED if !ignore_poisoning => {
// Panic to propagate the poison.
panic!("Once instance has previously been poisoned");
panic!("{}", super::ONCE_POISON_PANIC_MSG);
}
_ => {
// Set the QUEUED bit if it has not already been set.
Expand Down Expand Up @@ -147,7 +147,7 @@ impl Once {
COMPLETE => return,
POISONED if !ignore_poisoning => {
// Panic to propagate the poison.
panic!("Once instance has previously been poisoned");
panic!("{}", super::ONCE_POISON_PANIC_MSG);
}
INCOMPLETE | POISONED => {
// Try to register the current thread as the one running.
Expand Down
5 changes: 5 additions & 0 deletions library/std/src/sys/sync/once/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
// This also gives us the opportunity to optimize the implementation a bit which
// should help the fast path on call sites.

/// The panic message used when a `Once` is accessed after being poisoned.
///
/// This message is checked by `LazyLock` to detect when a `Once` has been poisoned.
pub(crate) const ONCE_POISON_PANIC_MSG: &str = "Once instance has previously been poisoned";

cfg_if::cfg_if! {
if #[cfg(any(
all(target_os = "windows", not(target_vendor="win7")),
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/sync/once/no_threads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl Once {
match state {
State::Poisoned if !ignore_poisoning => {
// Panic to propagate the poison.
panic!("Once instance has previously been poisoned");
panic!("{}", super::ONCE_POISON_PANIC_MSG);
}
State::Incomplete | State::Poisoned => {
self.state.set(State::Running);
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/sync/once/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ impl Once {
COMPLETE => return,
POISONED if !ignore_poisoning => {
// Panic to propagate the poison.
panic!("Once instance has previously been poisoned");
panic!("{}", super::ONCE_POISON_PANIC_MSG);
}
_ => {
current = wait(&self.state_and_queue, current, !ignore_poisoning);
Expand Down Expand Up @@ -189,7 +189,7 @@ impl Once {
COMPLETE => break,
POISONED if !ignore_poisoning => {
// Panic to propagate the poison.
panic!("Once instance has previously been poisoned");
panic!("{}", super::ONCE_POISON_PANIC_MSG);
}
POISONED | INCOMPLETE => {
// Try to register this thread as the one RUNNING.
Expand Down
29 changes: 29 additions & 0 deletions library/std/tests/sync/lazy_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,3 +165,32 @@ fn lazy_force_mut() {
p.clear();
LazyLock::force_mut(&mut lazy);
}

/// Verifies that when a `LazyLock` is poisoned, it panics with the correct error message ("LazyLock
/// instance has previously been poisoned") instead of the underlying `Once` error message.
#[test]
#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
#[should_panic(expected = "LazyLock instance has previously been poisoned")]
Copy link
Member

Choose a reason for hiding this comment

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

The reason this test passes is because #[should_panic(expected = "...")] likely checks the last panic message, not the first.

fn lazy_lock_poison_message() {
let lazy: LazyLock<String> = LazyLock::new(|| panic!("initialization failed"));

// First access will panic during initialization.
let _ = panic::catch_unwind(|| {
let _ = &*lazy;
});

// Second access should panic with the poisoned message.
let _ = &*lazy;
}

// Verifies that when the initialization closure panics with a custom message, that message is
// preserved and not overridden by `LazyLock`.
#[test]
#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
#[should_panic(expected = "custom panic message from closure")]
fn lazy_lock_preserves_closure_panic_message() {
let lazy: LazyLock<String> = LazyLock::new(|| panic!("custom panic message from closure"));

// This should panic with the original message from the closure.
let _ = &*lazy;
}
Loading