fix: Fix race condition on shutdown

crates/neon/src/object/mod.rs

@@ -36,7 +36,7 @@ use crate::{
     context::{internal::Env, Context},
     handle::{Handle, Managed, Root},
     result::{NeonResult, Throw},
-    sys::{self, raw, Status},
+    sys::{self, raw},
     types::{build, function::CallOptions, utf8::Utf8, JsFunction, JsUndefined, JsValue, Value},
 };
 
@@ -194,8 +194,8 @@ pub trait Object: Value {
         let obj = self.to_raw();
         unsafe {
             match sys::object::freeze(env, obj) {
-                Status::Ok => Ok(self),
-                Status::PendingException => Err(Throw::new()),
+                sys::Status::Ok => Ok(self),
+                sys::Status::PendingException => Err(Throw::new()),
                 _ => cx.throw_type_error("object cannot be frozen"),
             }
         }
@@ -207,8 +207,8 @@ pub trait Object: Value {
         let obj = self.to_raw();
         unsafe {
             match sys::object::seal(env, obj) {
-                Status::Ok => Ok(self),
-                Status::PendingException => Err(Throw::new()),
+                sys::Status::Ok => Ok(self),
+                sys::Status::PendingException => Err(Throw::new()),
                 _ => cx.throw_type_error("object cannot be sealed"),
             }
         }

crates/neon/src/sys/no_panic.rs

@@ -50,8 +50,8 @@ impl FailureBoundary {
     where
         F: FnOnce(Option<Env>) -> Local,
     {
-        // Event loop has terminated if `null`
-        let env = if env.is_null() { None } else { Some(env) };
+        // Make `env = None` if unable to call into JS
+        let env = can_call_into_js(env).then(|| env);
 
         // Run the user supplied callback, catching panics
         // This is unwind safe because control is never yielded back to the caller
@@ -121,6 +121,19 @@ impl FailureBoundary {
     }
 }
 
+// HACK: Force `NAPI_PREAMBLE` to run without executing any JavaScript to tell if it's
+// possible to call into JS.
+//
+// `NAPI_PREAMBLE` is a macro that checks if it is possible to call into JS.
+// https://github.com/nodejs/node/blob/5fad0b93667ffc6e4def52996b9529ac99b26319/src/js_native_api_v8.h#L211-L218
+//
+//  `napi_throw` starts by using `NAPI_PREAMBLE` and then a `CHECK_ARGS` on the `napi_value`. Since
+// we already know `env` is non-null, we expect the `null` value to cause a `napi_invalid_arg` error.
+// https://github.com/nodejs/node/blob/5fad0b93667ffc6e4def52996b9529ac99b26319/src/js_native_api_v8.cc#L1925-L1926
+fn can_call_into_js(env: Env) -> bool {
+    !env.is_null() && unsafe { napi::throw(env, ptr::null_mut()) == napi::Status::InvalidArg }
+}
+
 // We cannot use `napi_fatal_exception` because of this bug; instead, cause an
 // unhandled rejection which has similar behavior on recent versions of Node.
 // https://github.com/nodejs/node/issues/33771

test/napi/lib/workers.js

@@ -10,6 +10,11 @@ const addon = require("..");
 
 // Receive a message, try that method and return the error message
 if (!isMainThread) {
+  // RACE: Attempt to reproduce shutdown race condition bug. This depends on timings
+  // that may differ across systems. It should not produce spurious failures, but may
+  // succeed even if the presence of a bug.
+  addon.reject_after(new Error("Oh, no!"), 200).catch(() => {});
+
   addon.get_or_init_thread_id(threadId);
   parentPort.once("message", (message) => {
     try {
@@ -187,4 +192,11 @@ describe("Instance-local storage", () => {
 
     worker.postMessage("get_thread_id");
   });
+
+  it("should be able to exit a worker without a crash", (cb) => {
+    const worker = new Worker(__filename);
+
+    setTimeout(() => worker.terminate(), 50);
+    setTimeout(cb, 100);
+  });
 });

test/napi/src/js/workers.rs

@@ -1,4 +1,4 @@
-use std::sync::Mutex;
+use std::{convert::TryFrom, sync::Mutex, thread, time::Duration};
 
 use once_cell::sync::{Lazy, OnceCell};
 
@@ -90,3 +90,21 @@ pub fn unstash_global_object(mut cx: FunctionContext) -> JsResult<JsValue> {
         None => cx.null().upcast(),
     })
 }
+
+pub fn reject_after(mut cx: FunctionContext) -> JsResult<JsPromise> {
+    let err = cx.argument::<JsObject>(0)?.root(&mut cx);
+    let ms = cx.argument::<JsNumber>(1)?.value(&mut cx) as i64;
+    let ms = u64::try_from(ms)
+        .or_else(|err| cx.throw_error(err.to_string()))
+        .map(Duration::from_millis)?;
+
+    let promise =
+        cx.task(move || thread::sleep(ms))
+            .promise(move |mut cx, _| -> JsResult<JsValue> {
+                let err = err.into_inner(&mut cx);
+
+                cx.throw(err)
+            });
+
+    Ok(promise)
+}

test/napi/src/lib.rs

@@ -345,6 +345,7 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> {
     cx.export_function("get_reentrant_value", js::workers::get_reentrant_value)?;
     cx.export_function("stash_global_object", js::workers::stash_global_object)?;
     cx.export_function("unstash_global_object", js::workers::unstash_global_object)?;
+    cx.export_function("reject_after", js::workers::reject_after)?;
 
     Ok(())
 }