From 0a535872c78584985556b0e72ee4db8d33a906a4 Mon Sep 17 00:00:00 2001 From: Hana Date: Mon, 4 Dec 2023 13:08:27 +0800 Subject: [PATCH] feat: do not convert panics to results (#4848) * feat: do not convert panics to results * feat: add panic handler (#4849) --- Cargo.lock | 11 ++++ Cargo.toml | 6 ++- crates/node_binding/Cargo.toml | 2 + crates/node_binding/src/lib.rs | 20 +++---- crates/node_binding/src/loader.rs | 2 +- crates/node_binding/src/panic.rs | 52 +++++++++++++++++++ crates/rspack_binding_values/src/stats.rs | 2 +- crates/rspack_binding_values/src/utils.rs | 20 ++----- .../rspack_core/src/compiler/compilation.rs | 38 ++++---------- crates/rspack_napi_shared/src/errors.rs | 13 +++-- 10 files changed, 100 insertions(+), 66 deletions(-) create mode 100644 crates/node_binding/src/panic.rs diff --git a/Cargo.lock b/Cargo.lock index 8b0d8ec08e5..99a10fadb82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -421,6 +421,16 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "color-backtrace" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "150fd80a270c0671379f388c8204deb6a746bb4eac8a6c03fe2460b2c0127ea0" +dependencies = [ + "backtrace", + "termcolor", +] + [[package]] name = "colored" version = "2.0.4" @@ -2749,6 +2759,7 @@ name = "rspack_node" version = "0.1.0" dependencies = [ "async-trait", + "color-backtrace", "dashmap", "futures", "mimalloc-rust", diff --git a/Cargo.toml b/Cargo.toml index 1322be9235f..8204672d1fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,13 +68,15 @@ swc_node_comments = { version = "=0.20.9" } tikv-jemallocator = { version = "=0.5.4", features = ["disable_initial_exec_tls"] } [profile.dev] -codegen-units = 16 # debug build will cause runtime panic if codegen-unints is default +codegen-units = 16 # debug build will cause runtime panic if codegen-unints is default debug = 2 incremental = true +panic = "abort" [profile.release] codegen-units = 1 debug = false -lto = false # disabled by now, because it will significantly increase our compile time. +lto = false # disabled by now, because it will significantly increase our compile time. opt-level = 3 +panic = "abort" strip = true diff --git a/crates/node_binding/Cargo.toml b/crates/node_binding/Cargo.toml index dc877eb45ef..1f5317687ff 100644 --- a/crates/node_binding/Cargo.toml +++ b/crates/node_binding/Cargo.toml @@ -31,6 +31,8 @@ napi = { workspace = true } napi-derive = { workspace = true } napi-sys = { workspace = true } +color-backtrace = "0.6" + [target.'cfg(not(target_os = "linux"))'.dependencies] mimalloc-rust = { workspace = true } diff --git a/crates/node_binding/src/lib.rs b/crates/node_binding/src/lib.rs index 2975002d26a..86781665b4c 100644 --- a/crates/node_binding/src/lib.rs +++ b/crates/node_binding/src/lib.rs @@ -19,6 +19,7 @@ use rspack_napi_shared::NAPI_ENV; mod hook; mod loader; +mod panic; mod plugins; use hook::*; @@ -104,7 +105,6 @@ impl Rspack { #[allow(clippy::unwrap_in_result, clippy::unwrap_used)] #[napi( - catch_unwind, js_name = "unsafe_set_disabled_hooks", ts_args_type = "hooks: Array" )] @@ -119,7 +119,6 @@ impl Rspack { /// Warning: /// Calling this method recursively might cause a deadlock. #[napi( - catch_unwind, js_name = "unsafe_build", ts_args_type = "callback: (err: null | Error) => void" )] @@ -146,7 +145,6 @@ impl Rspack { /// Warning: /// Calling this method recursively will cause a deadlock. #[napi( - catch_unwind, js_name = "unsafe_rebuild", ts_args_type = "changed_files: string[], removed_files: string[], callback: (err: null | Error) => void" )] @@ -188,7 +186,7 @@ impl Rspack { /// Calling this method under the build or rebuild method might cause a deadlock. /// /// **Note** that this method is not safe if you cache the _JsCompilation_ on the Node side, as it will be invalidated by the next build and accessing a dangling ptr is a UB. - #[napi(catch_unwind, js_name = "unsafe_last_compilation")] + #[napi(js_name = "unsafe_last_compilation")] pub fn unsafe_last_compilation Result<()>>(&self, f: F) -> Result<()> { let handle_last_compilation = |compiler: &mut Pin>>| { // Safety: compiler is stored in a global hashmap, and compilation is only available in the callback of this function, so it is safe to cast to a static lifetime. See more in the warning part of this method. @@ -208,7 +206,7 @@ impl Rspack { /// Warning: /// /// Anything related to this compiler will be invalidated after this method is called. - #[napi(catch_unwind, js_name = "unsafe_drop")] + #[napi(js_name = "unsafe_drop")] pub fn drop(&self) -> Result<()> { unsafe { COMPILERS.remove(&self.id) }; @@ -236,8 +234,12 @@ enum TraceState { Off, } -static GLOBAL_TRACE_STATE: Lazy> = - Lazy::new(|| Mutex::new(TraceState::default())); +#[ctor] +fn init() { + panic::install_panic_handler(); +} + +static GLOBAL_TRACE_STATE: Mutex = Mutex::new(TraceState::Off); /** * Some code is modified based on @@ -246,7 +248,7 @@ static GLOBAL_TRACE_STATE: Lazy> = * Author Donny/강동윤 * Copyright (c) */ -#[napi(catch_unwind)] +#[napi] pub fn register_global_trace( filter: String, #[napi(ts_arg_type = "\"chrome\" | \"logger\"")] layer: String, @@ -269,7 +271,7 @@ pub fn register_global_trace( } } -#[napi(catch_unwind)] +#[napi] pub fn cleanup_global_trace() { let mut state = GLOBAL_TRACE_STATE .lock() diff --git a/crates/node_binding/src/loader.rs b/crates/node_binding/src/loader.rs index d4ebbecde9a..d7cff0edda0 100644 --- a/crates/node_binding/src/loader.rs +++ b/crates/node_binding/src/loader.rs @@ -2,7 +2,7 @@ use napi::Result; use rspack_binding_options::JsLoaderContext; /// Builtin loader runner -#[napi(catch_unwind)] +#[napi] #[allow(unused)] pub async fn run_builtin_loader( builtin: String, diff --git a/crates/node_binding/src/panic.rs b/crates/node_binding/src/panic.rs new file mode 100644 index 00000000000..d632d7a8dbd --- /dev/null +++ b/crates/node_binding/src/panic.rs @@ -0,0 +1,52 @@ +use color_backtrace::{default_output_stream, BacktracePrinter}; + +pub fn install_panic_handler() { + BacktracePrinter::default() + .message("Panic occurred at runtime. Please file an issue on GitHub with the backtrace below: https://github.com/web-infra-dev/rspack/issues") + .add_frame_filter(Box::new(|frames| { + static NAME_PREFIXES: &[&str] = &[ + "rust_panic", + "rayon", + "rust_begin_unwind", + "start_thread", + "__clone", + "call_once", + "catch_unwind", + "tokio", + " Option { self.inner.get_hash().map(|hash| hash.to_string()) } diff --git a/crates/rspack_binding_values/src/utils.rs b/crates/rspack_binding_values/src/utils.rs index 06b54de7858..93cd932f705 100644 --- a/crates/rspack_binding_values/src/utils.rs +++ b/crates/rspack_binding_values/src/utils.rs @@ -6,7 +6,6 @@ use dashmap::DashMap; use futures::Future; use napi::bindgen_prelude::*; use napi::{check_status, Env, Error, JsFunction, JsUnknown, NapiRaw, Result}; -use rspack_error::CatchUnwindFuture; use rspack_napi_shared::threadsafe_function::{ ThreadSafeContext, ThreadsafeFunction, ThreadsafeFunctionCallMode, }; @@ -102,23 +101,10 @@ where })?; napi::bindgen_prelude::spawn(async move { - let fut = CatchUnwindFuture::create(fut); let res = fut.await; - match res { - Ok(result) => { - tsfn - .call(result, ThreadsafeFunctionCallMode::NonBlocking) - .expect("Failed to call JS callback"); - } - Err(e) => { - tsfn - .call( - Err(Error::from_reason(format!("{e}"))), - ThreadsafeFunctionCallMode::NonBlocking, - ) - .expect("Failed to send panic info"); - } - } + tsfn + .call(res, ThreadsafeFunctionCallMode::NonBlocking) + .expect("Failed to call JS callback"); }); Ok(()) diff --git a/crates/rspack_core/src/compiler/compilation.rs b/crates/rspack_core/src/compiler/compilation.rs index 28a27ff550a..22bf7bd553d 100644 --- a/crates/rspack_core/src/compiler/compilation.rs +++ b/crates/rspack_core/src/compiler/compilation.rs @@ -14,9 +14,7 @@ use itertools::Itertools; use rayon::prelude::{ IntoParallelIterator, IntoParallelRefIterator, ParallelBridge, ParallelIterator, }; -use rspack_error::{ - internal_error, CatchUnwindFuture, Diagnostic, Result, Severity, TWithDiagnosticArray, -}; +use rspack_error::{internal_error, Diagnostic, Result, Severity, TWithDiagnosticArray}; use rspack_futures::FuturesResults; use rspack_hash::{RspackHash, RspackHashDigest}; use rspack_identifier::{Identifiable, IdentifierMap, IdentifierSet}; @@ -525,20 +523,11 @@ impl Compilation { return; } - let result = CatchUnwindFuture::create(task.run()).await; - - match result { - Ok(result) => { - if !is_expected_shutdown.load(Ordering::SeqCst) { - result_tx - .send(result) - .expect("Failed to send factorize result"); - } - } - Err(e) => { - // panic on the tokio worker thread - result_tx.send(Err(e)).expect("Failed to send panic info"); - } + let result = task.run().await; + if !is_expected_shutdown.load(Ordering::SeqCst) { + result_tx + .send(result) + .expect("Failed to send factorize result"); } } }); @@ -557,18 +546,9 @@ impl Compilation { return; } - let result = CatchUnwindFuture::create(task.run()).await; - - match result { - Ok(result) => { - if !is_expected_shutdown.load(Ordering::SeqCst) { - result_tx.send(result).expect("Failed to send build result"); - } - } - Err(e) => { - // panic on the tokio worker thread - result_tx.send(Err(e)).expect("Failed to send panic info"); - } + let result = task.run().await; + if !is_expected_shutdown.load(Ordering::SeqCst) { + result_tx.send(result).expect("Failed to send build result"); } } }); diff --git a/crates/rspack_napi_shared/src/errors.rs b/crates/rspack_napi_shared/src/errors.rs index 4ef66f5fafc..45e85be68af 100644 --- a/crates/rspack_napi_shared/src/errors.rs +++ b/crates/rspack_napi_shared/src/errors.rs @@ -39,9 +39,8 @@ impl NapiResultExt for Result { } } -#[inline(always)] -fn get_backtrace() -> String { - format!("{}", std::backtrace::Backtrace::force_capture()) +const fn get_backtrace() -> Option { + None } /// Extract stack or message from a native Node error object, @@ -55,14 +54,14 @@ fn extract_stack_or_message_from_napi_error(env: &Env, err: Error) -> (String, O match unsafe { ToNapiValue::to_napi_value(env.raw(), err) } { Ok(napi_error) => match try_extract_string_value_from_property(env, napi_error, "stack") { Err(_) => match try_extract_string_value_from_property(env, napi_error, "message") { - Err(e) => (format!("Unknown NAPI error {e}"), Some(get_backtrace())), - Ok(message) => (message, Some(get_backtrace())), + Err(e) => (format!("Unknown NAPI error {e}"), get_backtrace()), + Ok(message) => (message, get_backtrace()), }, - Ok(message) => (message, Some(get_backtrace())), + Ok(message) => (message, get_backtrace()), }, Err(e) => ( format!("Failed to extract NAPI error stack or message: {e}"), - Some(get_backtrace()), + get_backtrace(), ), } }