Skip to content

Commit 2ca31e4

Browse files
authored
Fall back on previous panic hook when not in catch_unwind wrapper (#15319)
This fixes #15317. Our `catch_unwind` wrapper installs a panic hook that captures (the rendered contents of) the panic info when a panic occurs. Since the intent is that the caller will render the panic info in some custom way, the hook silences the default stderr panic output. However, the panic hook is a global resource, so if any one thread was in the middle of a `catch_unwind` call, we would silence the default panic output for _all_ threads. The solution is to also keep a thread local that indicates whether the current thread is in the middle of our `catch_unwind`, and to fall back on the default panic hook if not. ## Test Plan Artificially added an mdtest parse error, ran tests via `cargo test -p red_knot_python_semantic` to run a large number of tests in parallel. Before this patch, the panic message was swallowed as reported in #15317. After, the panic message was shown.
1 parent 450d4e0 commit 2ca31e4

File tree

1 file changed

+53
-20
lines changed

1 file changed

+53
-20
lines changed

crates/ruff_db/src/panic.rs

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
use std::cell::Cell;
2+
use std::sync::OnceLock;
3+
14
#[derive(Default, Debug)]
25
pub struct PanicError {
36
pub info: String,
@@ -16,31 +19,61 @@ impl std::fmt::Display for PanicError {
1619
}
1720

1821
thread_local! {
19-
static LAST_PANIC: std::cell::Cell<Option<PanicError>> = const { std::cell::Cell::new(None) };
22+
static CAPTURE_PANIC_INFO: Cell<bool> = const { Cell::new(false) };
23+
static OUR_HOOK_RAN: Cell<bool> = const { Cell::new(false) };
24+
static LAST_PANIC: Cell<Option<PanicError>> = const { Cell::new(None) };
25+
}
26+
27+
fn install_hook() {
28+
static ONCE: OnceLock<()> = OnceLock::new();
29+
ONCE.get_or_init(|| {
30+
let prev = std::panic::take_hook();
31+
std::panic::set_hook(Box::new(move |info| {
32+
OUR_HOOK_RAN.with(|cell| cell.set(true));
33+
let should_capture = CAPTURE_PANIC_INFO.with(Cell::get);
34+
if !should_capture {
35+
return (*prev)(info);
36+
}
37+
let info = info.to_string();
38+
let backtrace = std::backtrace::Backtrace::force_capture();
39+
LAST_PANIC.with(|cell| {
40+
cell.set(Some(PanicError {
41+
info,
42+
backtrace: Some(backtrace),
43+
}));
44+
});
45+
}));
46+
});
2047
}
2148

22-
/// [`catch_unwind`](std::panic::catch_unwind) wrapper that sets a custom [`set_hook`](std::panic::set_hook)
23-
/// to extract the backtrace. The original panic-hook gets restored before returning.
49+
/// Invokes a closure, capturing and returning the cause of an unwinding panic if one occurs.
50+
///
51+
/// ### Thread safety
52+
///
53+
/// This is implemented by installing a custom [panic hook](std::panic::set_hook). This panic hook
54+
/// is a global resource. The hook that we install captures panic info in a thread-safe manner,
55+
/// and also ensures that any threads that are _not_ currently using this `catch_unwind` wrapper
56+
/// still use the previous hook (typically the default hook, which prints out panic information to
57+
/// stderr).
58+
///
59+
/// We assume that there is nothing else running in this process that needs to install a competing
60+
/// panic hook. We are careful to install our custom hook only once, and we do not ever restore
61+
/// the previous hook (since you can always retain the previous hook's behavior by not calling this
62+
/// wrapper).
2463
pub fn catch_unwind<F, R>(f: F) -> Result<R, PanicError>
2564
where
2665
F: FnOnce() -> R + std::panic::UnwindSafe,
2766
{
28-
let prev = std::panic::take_hook();
29-
std::panic::set_hook(Box::new(|info| {
30-
let info = info.to_string();
31-
let backtrace = std::backtrace::Backtrace::force_capture();
32-
LAST_PANIC.with(|cell| {
33-
cell.set(Some(PanicError {
34-
info,
35-
backtrace: Some(backtrace),
36-
}));
37-
});
38-
}));
39-
40-
let result = std::panic::catch_unwind(f)
41-
.map_err(|_| LAST_PANIC.with(std::cell::Cell::take).unwrap_or_default());
42-
43-
std::panic::set_hook(prev);
44-
67+
install_hook();
68+
OUR_HOOK_RAN.with(|cell| cell.set(false));
69+
let prev_should_capture = CAPTURE_PANIC_INFO.with(|cell| cell.replace(true));
70+
let result = std::panic::catch_unwind(f).map_err(|_| {
71+
let our_hook_ran = OUR_HOOK_RAN.with(Cell::get);
72+
if !our_hook_ran {
73+
panic!("detected a competing panic hook");
74+
}
75+
LAST_PANIC.with(Cell::take).unwrap_or_default()
76+
});
77+
CAPTURE_PANIC_INFO.with(|cell| cell.set(prev_should_capture));
4578
result
4679
}

0 commit comments

Comments
 (0)