From f9b7ebbff180ace224d1e6281354d484c3a0ec78 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Mon, 23 Sep 2024 19:16:35 +0200 Subject: [PATCH] [js] Avoid extraneous `eprintln!` invocation when callback is invoked from within a panic hook (#240) * Avoid extraneous `eprintln!` invocation when js callback is invoked from within a panic hook * Typo --- src/js/closures/common.rs | 2 ++ src/js/closures/node_js.rs | 49 ++++++++++++++++++++++---------------- src/js/closures/wasm.rs | 22 +++++++++-------- 3 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/js/closures/common.rs b/src/js/closures/common.rs index 770ea730b3..e53381a1ef 100644 --- a/src/js/closures/common.rs +++ b/src/js/closures/common.rs @@ -1,3 +1,5 @@ +use ::core::ops::Not as _; + #[derive(Debug)] pub struct ArcClosureRawParts { diff --git a/src/js/closures/node_js.rs b/src/js/closures/node_js.rs index ed82feabac..6ed2e4d45e 100644 --- a/src/js/closures/node_js.rs +++ b/src/js/closures/node_js.rs @@ -3,14 +3,14 @@ use super::*; use ::napi::threadsafe_function::*; +use ::once_cell::sync::OnceCell; use ::std::{ os::raw::c_void, sync::Arc, - time::Duration + time::Duration, }; -use once_cell::sync::OnceCell; -/// Global configuration for the number of milliseconds to wait for a callback sync. +/// Global configuration for the number of milliseconds to wait for a callback sync. /// Disabled when `None`. Access through `deadlock_timeout()`. static DEADLOCK_TIMEOUT_MS: OnceCell> = OnceCell::new(); @@ -561,14 +561,19 @@ macro_rules! impls {( $_k: $_k, )*)? ) -> CRet { - ::scopeguard::defer_on_unwind! { - eprintln!("\ - Attempted to panic through an `extern \"C\"` boundary, \ - which is undefined behavior. \ - Aborting for soundness.\ - "); - ::std::process::abort(); - } + // We set up an `on_unwind` guard, except if we're already being invoked + // from within a panicking context, which confuses the `on_unwind` heuristic + // of `::scopeguard`. Since in that case, any extra panic already triggers an abort, + // we can then just have the guard do nothing. + let _abort_on_unwind = ::std::thread::panicking().not().then(|| { + ::scopeguard::guard_on_unwind((), |()| { + eprintln! {"\ + Attempted to panic through the `extern \"C\"` boundary of a C `fn()`, \ + which is undefined behavior. Aborting for soundness.\ + "} + ::std::process::abort(); + }) + }); let &Self { ref ts_fun, @@ -696,19 +701,23 @@ macro_rules! impls {( ); debug_assert_eq!(status, Status::Ok); - let on_channel_closed = || panic!("Failed to receive a return value from a \ - callback function (channel closed): missing `wrap_cb_for_ffi` call?"); + let on_channel_closed = || panic! {"\ + Failed to receive a return value from a callback function \ + (channel closed): missing `wrap_cb_for_ffi` call?\ + "}; if let Some(timeout) = get_deadlock_timeout() { let duration = Duration::from_millis(timeout as u64); match ret_receiver.recv_timeout(duration) { Ok(ret) => ret, - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { - panic!("Failed to receive a return value from a callback \ - function (timeout after {}ms). This can be caused by a deadlock \ - or when execution is interrupted by an interactive debugger. \ - You can disable this check using \ - `Ditto.disableDeadlockDetection()", timeout) + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic! { + "\ + Failed to receive a return value from a callback function (timeout \ + after {}ms). This can be caused by a deadlock or when execution is \ + interrupted by an interactive debugger. You can disable this check \ + using `Ditto.disableDeadlockDetection()\ + ", + timeout, }, Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { on_channel_closed() @@ -843,7 +852,7 @@ fn dummy_ret_sender (env: &'_ Env) } /// Returns the configured deadlock timeout, or `None` if the timeout is disabled. -/// +/// /// The timeout can be set once by calling `set_deadlock_timeout`. If that has not happened, /// the default timeout is returned. pub fn get_deadlock_timeout() -> Option { diff --git a/src/js/closures/wasm.rs b/src/js/closures/wasm.rs index ae0d0cc0fd..069d786025 100644 --- a/src/js/closures/wasm.rs +++ b/src/js/closures/wasm.rs @@ -2,8 +2,8 @@ use super::*; -use ::{ - std::os::raw::c_void, +use ::std::{ + os::raw::c_void, }; use crate::{ layout::ReprC, @@ -152,14 +152,16 @@ match_! {( $_k: <$_k as ReprC>::CLayout )*)? ) -> ::CLayout { - ::scopeguard::defer_on_unwind! { - eprintln!("\ - Attempted to panic through an `extern \"C\"` boundary, \ - which is undefined behavior. \ - Aborting for soundness.\ - "); - ::std::process::abort(); - } + // Same logic as for `node_js`. + let _abort_on_unwind = ::std::thread::panicking().not().then(|| { + ::scopeguard::guard_on_unwind((), |()| { + eprintln! {"\ + Attempted to panic through the `extern \"C\"` boundary of a C `fn()`, \ + which is undefined behavior. Aborting for soundness.\ + "} + ::std::process::abort(); + }) + }); let &Self { ref js_fun,