fix(napi): improve error propagation (#1303)

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) },

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<T: FromNapiValue>(
     "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<T>>) };
   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
 }

crates/napi/src/env.rs

@@ -994,7 +994,7 @@ impl Env {
   }
 
   pub fn create_error(&self, e: Error) -> Result<JsObject> {
-    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 {

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<T> = std::result::Result<T, Error>;
 
@@ -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<T>)` scenario
+  maybe_raw: sys::napi_ref,
-  #[cfg(all(feature = "tokio_rt", feature = "napi4"))]
+  maybe_env: sys::napi_env,
-  pub(crate) maybe_raw: sys::napi_ref,
 }
 
 unsafe impl Send for Error {}
@@ -61,13 +60,18 @@ impl From<SerdeJSONError> for Error {
   }
 }
 
-#[cfg(all(feature = "tokio_rt", feature = "napi4"))]
+impl From<JsUnknown> for Error {
-impl From<sys::napi_ref> for Error {
+  fn from(value: JsUnknown) -> Self {
-  fn from(value: sys::napi_ref) -> 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<std::ffi::NulError> 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<std::io::Error> 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())),
+    }
+  }};
+}

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::<Vec<sys::napi_value>>();
     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::<Vec<sys::napi_value>>();
-    check_status!(unsafe {
+    check_pending_exception!(self.0.env, unsafe {
       sys::napi_new_instance(
         self.0.env,
         self.0.value,

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 {
+      let status =
-        sys::napi_reject_deferred(
+        unsafe { sys::napi_reject_deferred(env, deferred, JsError::from(e).into_value(env)) };
-          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
-          },
-        )
-      };
       debug_assert!(status == sys::Status::napi_ok, "Reject promise failed");
     }
   };

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')
+    },
+  )
+})

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<JsNull> {
@@ -32,6 +32,17 @@ pub fn call_function_with_this(ctx: CallContext) -> Result<JsNull> {
   ctx.env.get_null()
 }
 
+#[js_function(2)]
+pub fn call_function_error(ctx: CallContext) -> Result<JsUnknown> {
+  let js_func = ctx.get::<JsFunction>(0)?;
+  let error_func = ctx.get::<JsFunction>(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(())
 }