Skip to content
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

don't swallow panics from spawned threads #1763

Merged
Merged
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
25 changes: 17 additions & 8 deletions pgrx-pg-sys/src/submodules/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,14 +304,23 @@ fn take_panic_location() -> ErrorReportLocation {
}

pub fn register_pg_guard_panic_hook() {
std::panic::set_hook(Box::new(|info| {
PANIC_LOCATION.with(|thread_local| {
thread_local.replace({
let mut info: ErrorReportLocation = info.into();
info.backtrace = Some(std::backtrace::Backtrace::capture());
Some(info)
})
});
use super::thread_check::is_os_main_thread;

let default_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |info: _| {
if is_os_main_thread() == Some(true) {
// if this is the main thread, swallow the panic message and use postgres' error-reporting mechanism.
PANIC_LOCATION.with(|thread_local| {
thread_local.replace({
let mut info: ErrorReportLocation = info.into();
info.backtrace = Some(std::backtrace::Backtrace::capture());
Some(info)
})
});
} else {
// if this isn't the main thread, we don't know which connection to associate the panic with.
default_hook(info)
}
}))
}

Expand Down
4 changes: 2 additions & 2 deletions pgrx-pg-sys/src/submodules/thread_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub(crate) fn check_active_thread() {
/// Concretely, it is very important that this not return `Some(false)`
/// incorrectly, but the other values are less important. Callers generally
/// should compare the result against `Some(false)`.
fn is_os_main_thread() -> Option<bool> {
pub(super) fn is_os_main_thread() -> Option<bool> {
#[cfg(any(target_os = "macos", target_os = "openbsd", target_os = "freebsd"))]
return unsafe {
match libc::pthread_main_np() {
Expand Down Expand Up @@ -116,7 +116,7 @@ fn thread_id_check_failed() -> ! {
// I don't think this can ever happen, but it would be a bug if it could.
assert_ne!(is_os_main_thread(), Some(true), "`pgrx` active thread is not the main thread!?");
panic!(
"{}: postgres FFI may not not be called from multiple threads.",
"{}: postgres FFI may not be called from multiple threads.",
std::panic::Location::caller()
);
}
Expand Down
Loading