Skip to content

Commit

Permalink
feat(napi): keep stack traces in a deferred context (#1637)
Browse files Browse the repository at this point in the history
* feat(napi): keep stack traces in deferred context

* chore: reformat code

Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>

* chore: use napi wrappers

Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>

* test(napi): add test for deferred trace

Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>

* chore: fix format

Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>

---------

Signed-off-by: Markus <28785953+MarkusJx@users.noreply.github.com>
  • Loading branch information
MarkusJx committed Jul 15, 2023
1 parent f610129 commit 73a704a
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 5 deletions.
1 change: 1 addition & 0 deletions crates/napi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ tokio_stats = ["tokio/stats"]
tokio_sync = ["tokio/sync"]
tokio_test_util = ["tokio/test-util"]
tokio_time = ["tokio/time"]
deferred_trace = ["napi4"]

[dependencies]
ctor = "0.2"
Expand Down
130 changes: 125 additions & 5 deletions crates/napi/src/js_values/deferred.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,116 @@ use std::ptr;
use crate::bindgen_runtime::{ToNapiValue, THREAD_DESTROYED};
use crate::{check_status, JsError, JsObject, Value};
use crate::{sys, Env, Error, Result};
#[cfg(feature = "deferred_trace")]
use crate::{NapiRaw, NapiValue};

#[cfg(feature = "deferred_trace")]
/// A javascript error which keeps a stack trace
/// to the original caller in an asynchronous context.
/// This is required as the stack trace is lost when
/// an error is created in a different thread.
///
/// See this issue for more details:
/// https://github.com/nodejs/node-addon-api/issues/595
struct DeferredTrace {
value: sys::napi_ref,
#[cfg(not(feature = "noop"))]
env: sys::napi_env,
}

#[cfg(feature = "deferred_trace")]
impl DeferredTrace {
fn new(raw_env: sys::napi_env) -> Self {
let env = unsafe { Env::from_raw(raw_env) };
let reason = env.create_string("none").unwrap();

let mut js_error = ptr::null_mut();
let create_error_status =
unsafe { sys::napi_create_error(raw_env, ptr::null_mut(), reason.raw(), &mut js_error) };
debug_assert!(create_error_status == sys::Status::napi_ok);

let mut result = ptr::null_mut();
let status = unsafe { sys::napi_create_reference(raw_env, js_error, 1, &mut result) };
debug_assert!(status == sys::Status::napi_ok);

Self {
value: result,
#[cfg(not(feature = "noop"))]
env: raw_env,
}
}

fn into_rejected(self, raw_env: sys::napi_env, err: Error) -> Result<sys::napi_value> {
let env = unsafe { Env::from_raw(raw_env) };
let raw = unsafe { DeferredTrace::to_napi_value(raw_env, self)? };

let mut obj = unsafe { JsObject::from_raw(raw_env, raw)? };
if err.reason.is_empty() && err.status == crate::Status::GenericFailure {
// Can't clone err as the clone containing napi pointers will
// be freed when this function returns, causing err to be freed twice.
// Someone should probably fix this.
let err_obj = JsError::from(err).into_unknown(env).coerce_to_object()?;

if err_obj.has_named_property("message")? {
// The error was already created inside the JS engine, just return it
Ok(unsafe { JsError::from(Error::from(err_obj.into_unknown())).into_value(raw_env) })
} else {
obj.set_named_property("message", "")?;
obj.set_named_property("code", "")?;
Ok(raw)
}
} else {
obj.set_named_property("message", env.create_string(&err.reason)?)?;
obj.set_named_property("code", env.create_string_from_std(err.status.to_string())?)?;
Ok(raw)
}
}
}

#[cfg(feature = "deferred_trace")]
impl ToNapiValue for DeferredTrace {
unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
let mut value = ptr::null_mut();
check_status!(unsafe { sys::napi_get_reference_value(env, val.value, &mut value) })?;

if value.is_null() {
// This shouldn't happen but a panic is better than a segfault
Err(Error::new(
crate::Status::GenericFailure,
"Failed to get deferred error reference",
))
} else {
Ok(value)
}
}
}

#[cfg(feature = "deferred_trace")]
impl Drop for DeferredTrace {
fn drop(&mut self) {
#[cfg(not(feature = "noop"))]
{
if !self.env.is_null() && !self.value.is_null() {
let delete_reference_status = unsafe { sys::napi_delete_reference(self.env, self.value) };
debug_assert!(
delete_reference_status == sys::Status::napi_ok,
"Delete Error Reference failed"
);
}
}
}
}

struct DeferredData<Data: ToNapiValue, Resolver: FnOnce(Env) -> Result<Data>> {
resolver: Result<Resolver>,
#[cfg(feature = "deferred_trace")]
trace: DeferredTrace,
}

pub struct JsDeferred<Data: ToNapiValue, Resolver: FnOnce(Env) -> Result<Data>> {
tsfn: sys::napi_threadsafe_function,
#[cfg(feature = "deferred_trace")]
trace: DeferredTrace,
_data: PhantomData<Data>,
_resolver: PhantomData<Resolver>,
}
Expand Down Expand Up @@ -52,6 +159,8 @@ impl<Data: ToNapiValue, Resolver: FnOnce(Env) -> Result<Data>> JsDeferred<Data,

let deferred = Self {
tsfn,
#[cfg(feature = "deferred_trace")]
trace: DeferredTrace::new(env),
_data: PhantomData,
_resolver: PhantomData,
};
Expand All @@ -77,11 +186,17 @@ impl<Data: ToNapiValue, Resolver: FnOnce(Env) -> Result<Data>> JsDeferred<Data,
}

fn call_tsfn(self, result: Result<Resolver>) {
let data = DeferredData {
resolver: result,
#[cfg(feature = "deferred_trace")]
trace: self.trace,
};

// Call back into the JS thread via a threadsafe function. This results in napi_resolve_deferred being called.
let status = unsafe {
sys::napi_call_threadsafe_function(
self.tsfn,
Box::into_raw(Box::from(result)) as *mut c_void,
Box::into_raw(Box::from(data)) as *mut c_void,
sys::ThreadsafeFunctionCallMode::nonblocking,
)
};
Expand Down Expand Up @@ -113,8 +228,9 @@ extern "C" fn napi_resolve_deferred<Data: ToNapiValue, Resolver: FnOnce(Env) ->
}
}
let deferred = context as sys::napi_deferred;
let resolver = unsafe { Box::from_raw(data as *mut Result<Resolver>) };
let result = resolver
let deferred_data = unsafe { Box::from_raw(data as *mut DeferredData<Data, Resolver>) };
let result = deferred_data
.resolver
.and_then(|resolver| resolver(unsafe { Env::from_raw(env) }))
.and_then(|res| unsafe { ToNapiValue::to_napi_value(env, res) });

Expand All @@ -128,8 +244,12 @@ extern "C" fn napi_resolve_deferred<Data: ToNapiValue, Resolver: FnOnce(Env) ->
);
}
Err(e) => {
let status =
unsafe { sys::napi_reject_deferred(env, deferred, JsError::from(e).into_value(env)) };
#[cfg(feature = "deferred_trace")]
let error = deferred_data.trace.into_rejected(env, e).unwrap();
#[cfg(not(feature = "deferred_trace"))]
let error = unsafe { crate::JsError::from(e).into_value(env) };

let status = unsafe { sys::napi_reject_deferred(env, deferred, error) };
debug_assert!(
status == sys::Status::napi_ok,
"Reject promise failed {:?}",
Expand Down
1 change: 1 addition & 0 deletions examples/napi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ napi = { path = "../../crates/napi", default-features = false, features = [
"experimental",
"latin1",
"chrono_date",
"deferred_trace",
] }
napi-derive = { path = "../../crates/macro", features = ["type-def"] }
napi-shared = { path = "../napi-shared" }
Expand Down
2 changes: 2 additions & 0 deletions examples/napi/__tests__/__snapshots__/typegen.spec.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,8 @@ Generated by [AVA](https://avajs.dev).
export function threadsafeFunctionThrowError(cb: (...args: any[]) => any): void␊
export function throwAsyncError(): Promise<void>␊
export function throwError(): void␊
export function toJsObj(): object␊
Expand Down
Binary file modified examples/napi/__tests__/__snapshots__/typegen.spec.ts.snap
Binary file not shown.
8 changes: 8 additions & 0 deletions examples/napi/__tests__/values.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ import {
returnFromSharedCrate,
chronoNativeDateTime,
chronoNativeDateTimeReturn,
throwAsyncError,
} from '..'

test('export const', (t) => {
Expand Down Expand Up @@ -420,6 +421,13 @@ test('Result', (t) => {
}
})

test('Async error with stack trace', async (t) => {
const err = await t.throwsAsync(() => throwAsyncError())
t.not(err?.stack, undefined)
t.deepEqual(err!.message, 'Async Error')
t.regex(err!.stack!, /.+at .+values\.spec\.ts:\d+:\d+.+/gm)
})

test('custom status code in Error', (t) => {
t.throws(() => customStatusCode(), {
code: 'Panic',
Expand Down
2 changes: 2 additions & 0 deletions examples/napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,8 @@ export function threadsafeFunctionFatalModeError(cb: (...args: any[]) => any): v

export function threadsafeFunctionThrowError(cb: (...args: any[]) => any): void

export function throwAsyncError(): Promise<void>

export function throwError(): void

export function toJsObj(): object
Expand Down
5 changes: 5 additions & 0 deletions examples/napi/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@ impl AsRef<str> for CustomError {
pub fn custom_status_code() -> Result<(), CustomError> {
Err(Error::new(CustomError::Panic, "don't panic"))
}

#[napi]
pub async fn throw_async_error() -> Result<()> {
Err(Error::new(Status::InvalidArg, "Async Error".to_owned()))
}

0 comments on commit 73a704a

Please sign in to comment.