node-api: run Node-API finalizers in GC second pass

doc/api/n-api.md

@@ -5283,6 +5283,48 @@ invocation. If it is deleted before then, then the finalize callback may never
 be invoked. Therefore, when obtaining a reference a finalize callback is also
 required in order to enable correct disposal of the reference.
 
+### `node_api_set_finalizer_error_handler`
+
+<!-- YAML
+added: REPLACEME
+napiVersion: REPLACEME
+-->
+
+```c
+node_api_set_finalizer_error_handler(napi_env env,
+                                     napi_value error_handler);
+```
+
+* `[in] env`: The environment that the API is invoked under.
+* `[in] error_handler`: A JavaScript function that can handle JavaScript
+  exceptions thrown from a native finalizer.
+
+Returns `napi_ok` if the API succeeded.
+
+Sets or removes finalizer error handler which will be called when a native
+finalizer throws a JavaScript exception.
+
+By default any JavaScript exception from a native finalizer causes an unhandled
+exception that can be handled by a process-wide
+`process.on('uncaughtException', ...)` event handler.
+Otherwise, the unhandled exception causes the process termination.
+The `node_api_set_finalizer_error_handler` allows to set the error handler per
+`napi_env` instance which is created per native module instance, and to handle
+module-specific finalizer errors.
+
+The `error_handler` is a function with the following requirements:
+
+* the function receives a single paramter - the JavaScript error object;
+* the function can return `false` to indicate that the error was not handled and
+  to cause the unhandled exception;
+* if function returns any other value, then the exception is considered to
+  be handled;
+* if function throws a JavaScript exception, then it causes the the unhandled
+  exception.
+
+If the passed `error_handler` parameter is not a function, then the finalizer
+error handler is removed.
+
 ## Simple asynchronous operations
 
 Addon modules often need to leverage async helpers from libuv as part of their

src/js_native_api.h

@@ -497,6 +497,12 @@ NAPI_EXTERN napi_status napi_add_finalizer(napi_env env,
                                            void* finalize_hint,
                                            napi_ref* result);
 
+#ifdef NAPI_EXPERIMENTAL
+NAPI_EXTERN napi_status node_api_call_finalizers(napi_env env,
+                                                 size_t finalizer_count,
+                                                 bool* has_more_finalizers);
+#endif
+
 #endif  // NAPI_VERSION >= 5
 
 #if NAPI_VERSION >= 6
@@ -573,6 +579,12 @@ NAPI_EXTERN napi_status napi_object_seal(napi_env env,
                                          napi_value object);
 #endif  // NAPI_VERSION >= 8
 
+#ifdef NAPI_EXPERIMENTAL
+// Set a handler for JS errors in finalizers.
+NAPI_EXTERN napi_status
+node_api_set_finalizer_error_handler(napi_env env, napi_value error_handler);
+#endif  // NAPI_EXPERIMENTAL
+
 EXTERN_C_END
 
 #endif  // SRC_JS_NATIVE_API_H_

src/js_native_api_v8.cc

@@ -56,6 +56,54 @@
     (out) = v8::type::New((buffer), (byte_offset), (length));                  \
   } while (0)
 
+void napi_env__::HandleFinalizerException(napi_env env,
+                                          v8::Local<v8::Value> exception) {
+  // We must reset the last exception here to enable Node-API calls in the
+  // handler.
+  env->last_exception.Reset();
+  if (!env->finalizer_error_handler.IsEmpty()) {
+    bool isHandled = true;
+    napi_status status = [&]() {
+      v8::Local<v8::Value> v8handler =
+          v8::Local<v8::Value>::New(env->isolate, env->finalizer_error_handler);
+      napi_value handler = v8impl::JsValueFromV8LocalValue(v8handler);
+      napi_value err_value = v8impl::JsValueFromV8LocalValue(exception);
+      napi_value recv, result;
+      STATUS_CALL(napi_get_undefined(env, &recv));
+      STATUS_CALL(
+          napi_call_function(env, recv, handler, 1, &err_value, &result));
+      napi_valuetype result_type;
+      STATUS_CALL(napi_typeof(env, result, &result_type));
+      if (result_type == napi_boolean) {
+        bool bool_result;
+        STATUS_CALL(napi_get_value_bool(env, result, &bool_result));
+        if (bool_result == false) {
+          isHandled = false;
+        }
+      }
+      return napi_clear_last_error(env);
+    }();
+
+    if (status == napi_ok) {
+      if (!isHandled) {
+        env->isolate->ThrowException(exception);
+      }
+    } else if (status == napi_pending_exception) {
+      napi_value ex;
+      napi_get_and_clear_last_exception(env, &ex);
+      env->isolate->ThrowException(v8impl::V8LocalValueFromJsValue(ex));
+    } else {
+      const napi_extended_error_info* error_info;
+      napi_get_last_error_info(env, &error_info);
+      napi_throw_error(
+          env, "ERR_NAPI_FINALIZER_ERROR_HANDLER", error_info->error_message);
+      napi_clear_last_error(env);
+    }
+  } else {
+    env->isolate->ThrowException(exception);
+  }
+}
+
 namespace v8impl {
 
 namespace {
@@ -3295,3 +3343,23 @@ napi_status napi_is_detached_arraybuffer(napi_env env,
 
   return napi_clear_last_error(env);
 }
+
+NAPI_EXTERN napi_status
+node_api_set_finalizer_error_handler(napi_env env, napi_value error_handler) {
+  CHECK_ENV(env);
+
+  if (error_handler) {
+    v8::Local<v8::Value> handler =
+        v8impl::V8LocalValueFromJsValue(error_handler);
+    if (handler->IsFunction()) {
+      env->finalizer_error_handler =
+          v8impl::Persistent<v8::Value>(env->isolate, handler);
+    } else {
+      env->finalizer_error_handler.Reset();
+    }
+  } else {
+    env->finalizer_error_handler.Reset();
+  }
+
+  return napi_clear_last_error(env);
+}

src/js_native_api_v8.h

@@ -102,11 +102,13 @@ struct napi_env__ {
     }
   }
 
+  static void HandleFinalizerException(napi_env env,
+                                       v8::Local<v8::Value> exception);
+
   virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
     v8::HandleScope handle_scope(isolate);
-    CallIntoModule([&](napi_env env) {
-      cb(env, data, hint);
-    });
+    CallIntoModule([&](napi_env env) { cb(env, data, hint); },
+                   HandleFinalizerException);
   }
 
   v8impl::Persistent<v8::Value> last_exception;
@@ -121,6 +123,7 @@ struct napi_env__ {
   int open_callback_scopes = 0;
   int refs = 1;
   void* instance_data = nullptr;
+  v8impl::Persistent<v8::Value> finalizer_error_handler;
 };
 
 // This class is used to keep a napi_env live in a way that

src/node_api.cc

@@ -34,17 +34,18 @@ v8::Maybe<bool> node_napi_env__::mark_arraybuffer_as_untransferable(
 }
 
 void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
-  // we need to keep the env live until the finalizer has been run
-  // EnvRefHolder provides an exception safe wrapper to Ref and then
-  // Unref once the lambda is freed
-  EnvRefHolder liveEnv(static_cast<napi_env>(this));
-  node_env()->SetImmediate(
-      [=, liveEnv = std::move(liveEnv)](node::Environment* node_env) {
-        napi_env env = liveEnv.env();
-        v8::HandleScope handle_scope(env->isolate);
-        v8::Context::Scope context_scope(env->context());
-        env->CallIntoModule([&](napi_env env) { cb(env, data, hint); });
-      });
+  // Handle JS exceptions from finalizers the same way as we do it in the
+  // SetImmediate. See Environment::RunAndClearNativeImmediates.
+  node::errors::TryCatchScope try_catch(node_env());
+  node::DebugSealHandleScope seal_handle_scope(isolate);
+
+  napi_env__::CallFinalizer(cb, data, hint);
+
+  if (UNLIKELY(try_catch.HasCaught())) {
+    if (!try_catch.HasTerminated() && can_call_into_js()) {
+      node::errors::TriggerUncaughtException(isolate, try_catch);
+    }
+  }
 }
 
 namespace v8impl {

test/js-native-api/test_finalizer_exception/binding.gyp

@@ -0,0 +1,12 @@
+{
+  "targets": [
+    {
+      "target_name": "test_finalizer_exception",
+      "sources": [
+        "../entry_point.c",
+        "testobject.cc",
+        "test_finalizer_exception.cc"
+      ]
+    }
+  ]
+}

test/js-native-api/test_finalizer_exception/test_finalizer_exception.cc

@@ -0,0 +1,51 @@
+#define NAPI_EXPERIMENTAL
+#include <js_native_api.h>
+#include "../common.h"
+#include "testobject.h"
+
+napi_value CreateObject(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value args[1] = {};
+  NODE_API_CALL(env,
+                napi_get_cb_info(env, info, &argc, args, nullptr, nullptr));
+
+  napi_value instance;
+  NODE_API_CALL(env, TestObject::NewInstance(env, args[0], &instance));
+
+  return instance;
+}
+
+napi_value SetFinalizerErrorHandler(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value args[1] = {};
+  NODE_API_CALL(env,
+                napi_get_cb_info(env, info, &argc, args, nullptr, nullptr));
+
+  NODE_API_CALL(env, node_api_set_finalizer_error_handler(env, args[0]));
+
+  napi_value undefined;
+  NODE_API_CALL(env, napi_get_undefined(env, &undefined));
+  return undefined;
+}
+
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
+  NODE_API_CALL(env, TestObject::Init(env));
+
+  napi_property_descriptor descriptors[] = {
+      DECLARE_NODE_API_GETTER("finalizeCount", TestObject::GetFinalizeCount),
+      DECLARE_NODE_API_PROPERTY("createObject", CreateObject),
+      DECLARE_NODE_API_PROPERTY("setFinalizerErrorHandler",
+                                SetFinalizerErrorHandler),
+  };
+
+  NODE_API_CALL(
+      env,
+      napi_define_properties(env,
+                             exports,
+                             sizeof(descriptors) / sizeof(*descriptors),
+                             descriptors));
+
+  return exports;
+}
+EXTERN_C_END

test/js-native-api/test_finalizer_exception/test_handled_exception.js

@@ -0,0 +1,81 @@
+'use strict';
+// Flags: --expose-gc
+
+// The test verifies that if finalizer throws an exception,
+// then it can be handled by custom finalizer error handler.
+// If it is not handled in the handler, causing a
+// Node.js uncaughtException.
+
+const common = require('../../common');
+const assert = require('assert');
+const test = require(`./build/${common.buildType}/test_finalizer_exception`);
+
+assert.strictEqual(test.finalizeCount, 0);
+
+function gcUntilSync(value) {
+  for (let i = 0; i < 10; ++i) {
+    global.gc();
+    if (test.finalizeCount === value) {
+      break;
+    }
+  }
+}
+
+function runGCTests() {
+  let unhandledExceptions = 0;
+  process.on('uncaughtException', (err) => {
+    ++unhandledExceptions;
+    assert.strictEqual(err.message, 'Error during Finalize');
+  });
+
+  let handledExceptions = 0;
+  test.setFinalizerErrorHandler((err) => {
+    ++handledExceptions;
+    assert.strictEqual(err.message, 'Error during Finalize');
+    if (handledExceptions === 2) {
+      // One time we report the error to be unhandled
+      return false;
+    }
+  });
+
+  (() => {
+    test.createObject(true /* throw on destruct */);
+  })();
+  gcUntilSync(1);
+  assert.strictEqual(test.finalizeCount, 1);
+  assert.strictEqual(handledExceptions, 1);
+  assert.strictEqual(unhandledExceptions, 0);
+
+  (() => {
+    test.createObject(true /* throw on destruct */);
+    test.createObject(true /* throw on destruct */);
+  })();
+  gcUntilSync(3);
+  assert.strictEqual(test.finalizeCount, 3);
+  assert.strictEqual(handledExceptions, 3);
+  assert.strictEqual(unhandledExceptions, 1);
+
+  test.setFinalizerErrorHandler(null);
+  (() => {
+    test.createObject(true /* throw on destruct */);
+  })();
+  gcUntilSync(4);
+  assert.strictEqual(test.finalizeCount, 4);
+  assert.strictEqual(handledExceptions, 3);
+  assert.strictEqual(unhandledExceptions, 2);
+
+  // Raise unhandled exception if finalizer error handler throws.
+  test.setFinalizerErrorHandler((err) => {
+    ++handledExceptions;
+    assert.strictEqual(err.message, 'Error during Finalize');
+    throw err;
+  });
+  (() => {
+    test.createObject(true /* throw on destruct */);
+  })();
+  gcUntilSync(5);
+  assert.strictEqual(test.finalizeCount, 5);
+  assert.strictEqual(handledExceptions, 4);
+  assert.strictEqual(unhandledExceptions, 3);
+}
+runGCTests();

test/js-native-api/test_finalizer_exception/test_no_exception.js

@@ -0,0 +1,36 @@
+'use strict';
+// Flags: --expose-gc
+
+// The test verifies that finalizers are called as part of the main script
+// execution when there are no exceptions.
+
+const common = require('../../common');
+const assert = require('assert');
+const test = require(`./build/${common.buildType}/test_finalizer_exception`);
+
+assert.strictEqual(test.finalizeCount, 0);
+
+function gcUntilSync(value) {
+  for (let i = 0; i < 10; ++i) {
+    global.gc();
+    if (test.finalizeCount === value) {
+      break;
+    }
+  }
+}
+
+function runGCTests() {
+  (() => {
+    test.createObject();
+  })();
+  gcUntilSync(1);
+  assert.strictEqual(test.finalizeCount, 1);
+
+  (() => {
+    test.createObject();
+    test.createObject();
+  })();
+  gcUntilSync(3);
+  assert.strictEqual(test.finalizeCount, 3);
+}
+runGCTests();