From 5ba70b0e1a64a7f68e30d472a4ac107519d1dd0d Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Wed, 14 Sep 2022 02:03:11 -0700 Subject: [PATCH] fix(napi): improve error propagation (#1303) --- crates/napi/src/bindgen_runtime/js_values.rs | 2 +- .../src/bindgen_runtime/js_values/promise.rs | 13 +-- crates/napi/src/env.rs | 2 +- crates/napi/src/error.rs | 85 +++++++++++++++---- crates/napi/src/js_values/function.rs | 8 +- crates/napi/src/promise.rs | 24 +----- .../__test__/function.spec.ts | 11 +++ examples/napi-compat-mode/src/function.rs | 14 ++- 8 files changed, 106 insertions(+), 53 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/js_values.rs b/crates/napi/src/bindgen_runtime/js_values.rs index 3b8d175f..7577b97a 100644 --- a/crates/napi/src/bindgen_runtime/js_values.rs +++ b/crates/napi/src/bindgen_runtime/js_values.rs @@ -233,7 +233,7 @@ where Ok(v) => unsafe { T::to_napi_value(env, v) }, Err(e) => { let error_code = unsafe { String::to_napi_value(env, format!("{:?}", e.status))? }; - let reason = unsafe { String::to_napi_value(env, e.reason)? }; + let reason = unsafe { String::to_napi_value(env, e.reason.clone())? }; let mut error = ptr::null_mut(); check_status!( unsafe { sys::napi_create_error(env, error_code, reason, &mut error) }, diff --git a/crates/napi/src/bindgen_runtime/js_values/promise.rs b/crates/napi/src/bindgen_runtime/js_values/promise.rs index b2b710b0..9beea5d0 100644 --- a/crates/napi/src/bindgen_runtime/js_values/promise.rs +++ b/crates/napi/src/bindgen_runtime/js_values/promise.rs @@ -6,7 +6,7 @@ use std::task::{Context, Poll}; use tokio::sync::oneshot::{channel, Receiver, Sender}; -use crate::{check_status, sys, Error, Result, Status}; +use crate::{check_status, sys, Error, JsUnknown, NapiValue, Result, Status}; use super::{FromNapiValue, TypeName, ValidateNapiValue}; @@ -224,16 +224,11 @@ unsafe extern "C" fn catch_callback( "Get callback info from Promise::catch failed" ); let rejected_value = rejected_value[0]; - let mut error_ref = ptr::null_mut(); - let create_ref_status = - unsafe { sys::napi_create_reference(env, rejected_value, 1, &mut error_ref) }; - debug_assert!( - create_ref_status == sys::Status::napi_ok, - "Create Error reference failed" - ); let sender = unsafe { Box::from_raw(data as *mut Sender<*mut Result>) }; sender - .send(Box::into_raw(Box::new(Err(Error::from(error_ref))))) + .send(Box::into_raw(Box::new(Err(Error::from(unsafe { + JsUnknown::from_raw_unchecked(env, rejected_value) + }))))) .expect("Send Promise resolved value error"); this } diff --git a/crates/napi/src/env.rs b/crates/napi/src/env.rs index 88b6a1d4..e5ab7b6c 100644 --- a/crates/napi/src/env.rs +++ b/crates/napi/src/env.rs @@ -994,7 +994,7 @@ impl Env { } pub fn create_error(&self, e: Error) -> Result { - let reason = e.reason; + let reason = &e.reason; let reason_string = self.create_string(reason.as_str())?; let mut result = ptr::null_mut(); check_status!(unsafe { diff --git a/crates/napi/src/error.rs b/crates/napi/src/error.rs index d6d9e837..de37b6ef 100644 --- a/crates/napi/src/error.rs +++ b/crates/napi/src/error.rs @@ -12,7 +12,7 @@ use serde::{de, ser}; #[cfg(feature = "serde-json")] use serde_json::Error as SerdeJSONError; -use crate::{check_status, sys, Status}; +use crate::{check_status, sys, Env, JsUnknown, NapiValue, Status}; pub type Result = std::result::Result; @@ -24,9 +24,8 @@ pub struct Error { pub status: Status, pub reason: String, // Convert raw `JsError` into Error - // Only be used in `async fn(p: Promise)` scenario - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] - pub(crate) maybe_raw: sys::napi_ref, + maybe_raw: sys::napi_ref, + maybe_env: sys::napi_env, } unsafe impl Send for Error {} @@ -61,13 +60,18 @@ impl From for Error { } } -#[cfg(all(feature = "tokio_rt", feature = "napi4"))] -impl From for Error { - fn from(value: sys::napi_ref) -> Self { +impl From for Error { + fn from(value: JsUnknown) -> Self { + let mut result = std::ptr::null_mut(); + let status = unsafe { sys::napi_create_reference(value.0.env, value.0.value, 0, &mut result) }; + if status != sys::Status::napi_ok { + return Error::new(Status::from(status), "".to_owned()); + } Self { - status: Status::InvalidArg, + status: Status::GenericFailure, reason: "".to_string(), - maybe_raw: value, + maybe_raw: result, + maybe_env: value.0.env, } } } @@ -94,8 +98,8 @@ impl Error { Error { status, reason, - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] maybe_raw: ptr::null_mut(), + maybe_env: ptr::null_mut(), } } @@ -103,8 +107,8 @@ impl Error { Error { status, reason: "".to_owned(), - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] maybe_raw: ptr::null_mut(), + maybe_env: ptr::null_mut(), } } @@ -112,8 +116,8 @@ impl Error { Error { status: Status::GenericFailure, reason: reason.into(), - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] maybe_raw: ptr::null_mut(), + maybe_env: ptr::null_mut(), } } } @@ -123,8 +127,8 @@ impl From for Error { Error { status: Status::GenericFailure, reason: format!("{}", error), - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] maybe_raw: ptr::null_mut(), + maybe_env: ptr::null_mut(), } } } @@ -134,8 +138,21 @@ impl From for Error { Error { status: Status::GenericFailure, reason: format!("{}", error), - #[cfg(all(feature = "tokio_rt", feature = "napi4"))] maybe_raw: ptr::null_mut(), + maybe_env: ptr::null_mut(), + } + } +} + +impl Drop for Error { + fn drop(&mut self) { + if !self.maybe_env.is_null() && !self.maybe_raw.is_null() { + let delete_reference_status = + unsafe { sys::napi_delete_reference(self.maybe_env, self.maybe_raw) }; + debug_assert!( + delete_reference_status == sys::Status::napi_ok, + "Delete Error Reference failed" + ); } } } @@ -188,11 +205,22 @@ macro_rules! impl_object_methods { /// /// This function is safety if env is not null ptr. pub unsafe fn into_value(self, env: sys::napi_env) -> sys::napi_value { + if !self.0.maybe_raw.is_null() { + let mut err = ptr::null_mut(); + let get_err_status = + unsafe { sys::napi_get_reference_value(env, self.0.maybe_raw, &mut err) }; + debug_assert!( + get_err_status == sys::Status::napi_ok, + "Get Error from Reference failed" + ); + return err; + } + let error_status = format!("{:?}", self.0.status); let status_len = error_status.len(); let error_code_string = CString::new(error_status).unwrap(); let reason_len = self.0.reason.len(); - let reason = CString::new(self.0.reason).unwrap(); + let reason = CString::new(self.0.reason.as_str()).unwrap(); let mut error_code = ptr::null_mut(); let mut reason_string = ptr::null_mut(); let mut js_error = ptr::null_mut(); @@ -209,6 +237,11 @@ macro_rules! impl_object_methods { js_error } + pub fn into_unknown(self, env: Env) -> JsUnknown { + let value = unsafe { self.into_value(env.raw()) }; + unsafe { JsUnknown::from_raw_unchecked(env.raw(), value) } + } + /// # Safety /// /// This function is safety if env is not null ptr. @@ -297,3 +330,25 @@ macro_rules! check_status { } }}; } + +#[doc(hidden)] +#[macro_export] +macro_rules! check_pending_exception { + ($env: expr, $code:expr) => {{ + let c = $code; + match c { + $crate::sys::Status::napi_ok => Ok(()), + $crate::sys::Status::napi_pending_exception => { + let mut error_result = std::ptr::null_mut(); + assert_eq!( + unsafe { $crate::sys::napi_get_and_clear_last_exception($env, &mut error_result) }, + $crate::sys::Status::napi_ok + ); + return Err(Error::from(unsafe { + JsUnknown::from_raw_unchecked($env, error_result) + })); + } + _ => Err($crate::Error::new($crate::Status::from(c), "".to_owned())), + } + }}; +} diff --git a/crates/napi/src/js_values/function.rs b/crates/napi/src/js_values/function.rs index f6dfa5f6..01c777b2 100644 --- a/crates/napi/src/js_values/function.rs +++ b/crates/napi/src/js_values/function.rs @@ -7,7 +7,7 @@ use crate::{ bindgen_runtime::ToNapiValue, threadsafe_function::{ThreadSafeCallContext, ThreadsafeFunction}, }; -use crate::{check_status, ValueType}; +use crate::{check_pending_exception, ValueType}; use crate::{sys, Env, Error, JsObject, JsUnknown, NapiRaw, NapiValue, Result, Status}; pub struct JsFunction(pub(crate) Value); @@ -56,7 +56,7 @@ impl JsFunction { .map(|arg| unsafe { arg.raw() }) .collect::>(); let mut return_value = ptr::null_mut(); - check_status!(unsafe { + check_pending_exception!(self.0.env, unsafe { sys::napi_call_function( self.0.env, raw_this, @@ -83,7 +83,7 @@ impl JsFunction { }) .ok_or_else(|| Error::new(Status::GenericFailure, "Get raw this failed".to_owned()))?; let mut return_value = ptr::null_mut(); - check_status!(unsafe { + check_pending_exception!(self.0.env, unsafe { sys::napi_call_function( self.0.env, raw_this, @@ -110,7 +110,7 @@ impl JsFunction { .iter() .map(|arg| unsafe { arg.raw() }) .collect::>(); - check_status!(unsafe { + check_pending_exception!(self.0.env, unsafe { sys::napi_new_instance( self.0.env, self.0.value, diff --git a/crates/napi/src/promise.rs b/crates/napi/src/promise.rs index 3c93f3db..ae8dd5cf 100644 --- a/crates/napi/src/promise.rs +++ b/crates/napi/src/promise.rs @@ -109,28 +109,8 @@ unsafe extern "C" fn call_js_cb< debug_assert!(status == sys::Status::napi_ok, "Resolve promise failed"); } Err(e) => { - let status = unsafe { - sys::napi_reject_deferred( - env, - deferred, - if e.maybe_raw.is_null() { - JsError::from(e).into_value(env) - } else { - let mut err = ptr::null_mut(); - let get_err_status = sys::napi_get_reference_value(env, e.maybe_raw, &mut err); - debug_assert!( - get_err_status == sys::Status::napi_ok, - "Get Error from Reference failed" - ); - let delete_reference_status = sys::napi_delete_reference(env, e.maybe_raw); - debug_assert!( - delete_reference_status == sys::Status::napi_ok, - "Delete Error Reference failed" - ); - err - }, - ) - }; + let status = + unsafe { sys::napi_reject_deferred(env, deferred, JsError::from(e).into_value(env)) }; debug_assert!(status == sys::Status::napi_ok, "Reject promise failed"); } }; diff --git a/examples/napi-compat-mode/__test__/function.spec.ts b/examples/napi-compat-mode/__test__/function.spec.ts index 0dd4f4ec..5dcfd346 100644 --- a/examples/napi-compat-mode/__test__/function.spec.ts +++ b/examples/napi-compat-mode/__test__/function.spec.ts @@ -20,3 +20,14 @@ test('should set "this" properly', (t) => { t.is(this, obj) }) }) + +test('should handle errors', (t) => { + bindings.testCallFunctionError( + () => { + throw new Error('Testing') + }, + (err: Error) => { + t.is(err.message, 'Testing') + }, + ) +}) diff --git a/examples/napi-compat-mode/src/function.rs b/examples/napi-compat-mode/src/function.rs index 1582128d..ff04aac1 100644 --- a/examples/napi-compat-mode/src/function.rs +++ b/examples/napi-compat-mode/src/function.rs @@ -1,4 +1,4 @@ -use napi::{CallContext, JsFunction, JsNull, JsObject, Result}; +use napi::{CallContext, JsError, JsFunction, JsNull, JsObject, JsUnknown, Result}; #[js_function(1)] pub fn call_function(ctx: CallContext) -> Result { @@ -32,6 +32,17 @@ pub fn call_function_with_this(ctx: CallContext) -> Result { ctx.env.get_null() } +#[js_function(2)] +pub fn call_function_error(ctx: CallContext) -> Result { + let js_func = ctx.get::(0)?; + let error_func = ctx.get::(1)?; + + match js_func.call_without_args(None) { + Ok(v) => Ok(v), + Err(e) => error_func.call(None, &[JsError::from(e).into_unknown(*ctx.env)]), + } +} + pub fn register_js(exports: &mut JsObject) -> Result<()> { exports.create_named_method("testCallFunction", call_function)?; exports.create_named_method( @@ -39,5 +50,6 @@ pub fn register_js(exports: &mut JsObject) -> Result<()> { call_function_with_ref_arguments, )?; exports.create_named_method("testCallFunctionWithThis", call_function_with_this)?; + exports.create_named_method("testCallFunctionError", call_function_error)?; Ok(()) }