Skip to content

Commit

Permalink
feat: do not convert panics to results (#4848)
Browse files Browse the repository at this point in the history
* feat: do not convert panics to results

* feat: add panic handler (#4849)
  • Loading branch information
h-a-n-a authored Dec 4, 2023
1 parent 1dd81be commit 0a53587
Show file tree
Hide file tree
Showing 10 changed files with 100 additions and 66 deletions.
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 2 additions & 0 deletions crates/node_binding/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down
20 changes: 11 additions & 9 deletions crates/node_binding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rspack_napi_shared::NAPI_ENV;

mod hook;
mod loader;
mod panic;
mod plugins;

use hook::*;
Expand Down Expand Up @@ -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<string>"
)]
Expand All @@ -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"
)]
Expand All @@ -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"
)]
Expand Down Expand Up @@ -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<F: Fn(JsCompilation) -> Result<()>>(&self, f: F) -> Result<()> {
let handle_last_compilation = |compiler: &mut Pin<Box<rspack_core::Compiler<_>>>| {
// 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.
Expand All @@ -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) };

Expand Down Expand Up @@ -236,8 +234,12 @@ enum TraceState {
Off,
}

static GLOBAL_TRACE_STATE: Lazy<Mutex<TraceState>> =
Lazy::new(|| Mutex::new(TraceState::default()));
#[ctor]
fn init() {
panic::install_panic_handler();
}

static GLOBAL_TRACE_STATE: Mutex<TraceState> = Mutex::new(TraceState::Off);

/**
* Some code is modified based on
Expand All @@ -246,7 +248,7 @@ static GLOBAL_TRACE_STATE: Lazy<Mutex<TraceState>> =
* Author Donny/강동윤
* Copyright (c)
*/
#[napi(catch_unwind)]
#[napi]
pub fn register_global_trace(
filter: String,
#[napi(ts_arg_type = "\"chrome\" | \"logger\"")] layer: String,
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion crates/node_binding/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
52 changes: 52 additions & 0 deletions crates/node_binding/src/panic.rs
Original file line number Diff line number Diff line change
@@ -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",
"<tokio::runtime",
"future",
"std::panic",
"<core::panic",
"___rust_try",
];
static FILE_PREFIXES: &[&str] = &[
"/rustc/",
"src/libstd/",
"src/libpanic_unwind/",
"src/libtest/",
];
frames.retain(|x| {
if x.is_post_panic_code() || x.is_runtime_init_code() {
return false;
}

if matches!(&x.filename, Some(f) if FILE_PREFIXES.iter().any(|l| {
f.starts_with(l)
})) {
return false;
}

if matches!(&x.name, Some(n) if NAME_PREFIXES.iter().any(|l| {
n.starts_with(l)
})) {
return false;
}

true
});
}))
.verbosity(color_backtrace::Verbosity::Medium)
.lib_verbosity(color_backtrace::Verbosity::Medium)
.print_addresses(false)
.install(default_output_stream());
}
2 changes: 1 addition & 1 deletion crates/rspack_binding_values/src/stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ impl JsStats {
.collect()
}

#[napi(catch_unwind)]
#[napi]
pub fn get_hash(&self) -> Option<String> {
self.inner.get_hash().map(|hash| hash.to_string())
}
Expand Down
20 changes: 3 additions & 17 deletions crates/rspack_binding_values/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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(())
Expand Down
38 changes: 9 additions & 29 deletions crates/rspack_core/src/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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");
}
}
});
Expand All @@ -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");
}
}
});
Expand Down
13 changes: 6 additions & 7 deletions crates/rspack_napi_shared/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ impl<T: 'static> NapiResultExt<T> for Result<T> {
}
}

#[inline(always)]
fn get_backtrace() -> String {
format!("{}", std::backtrace::Backtrace::force_capture())
const fn get_backtrace() -> Option<String> {
None
}

/// Extract stack or message from a native Node error object,
Expand All @@ -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(),
),
}
}
Expand Down

0 comments on commit 0a53587

Please sign in to comment.