node-api: enable uncaught exceptions policy by default

This enables the option `--force-node-api-uncaught-exceptions-policy` for a specific Node-API addon when it is compiled with `NAPI_EXPERIMENTAL` (and this would be the default behavior when `NAPI_VERSION` 10 releases). This would not break existing Node-API addons. PR-URL: nodejs/node#49313 Refs: nodejs/node#36510 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
sercher · Apr 25, 2024 · 2bba1c9 · 2bba1c9
1 parent 4c6f92f
commit 2bba1c9
Show file tree

Hide file tree

Showing 16 changed files with 307 additions and 133 deletions.
diff --git a/graal-nodejs/doc/api/n-api.md b/graal-nodejs/doc/api/n-api.md
@@ -6241,6 +6241,13 @@ napi_create_threadsafe_function(napi_env env,
   [`napi_threadsafe_function_call_js`][] provides more details.
 * `[out] result`: The asynchronous thread-safe JavaScript function.
 
+**Change History:**
+
+* Experimental (`NAPI_EXPERIMENTAL` is defined):
+
+  Uncaught exceptions thrown in `call_js_cb` are handled with the
+  [`'uncaughtException'`][] event, instead of being ignored.
+
 ### `napi_get_threadsafe_function_context`
 
 <!-- YAML
@@ -6471,6 +6478,7 @@ the add-on's file name during loading.
 [Visual Studio]: https://visualstudio.microsoft.com
 [Working with JavaScript properties]: #working-with-javascript-properties
 [Xcode]: https://developer.apple.com/xcode/
+[`'uncaughtException'`]: process.md#event-uncaughtexception
 [`Number.MAX_SAFE_INTEGER`]: https://tc39.github.io/ecma262/#sec-number.max_safe_integer
 [`Number.MIN_SAFE_INTEGER`]: https://tc39.github.io/ecma262/#sec-number.min_safe_integer
 [`Worker`]: worker_threads.md#class-worker

diff --git a/graal-nodejs/src/node_api.cc b/graal-nodejs/src/node_api.cc
@@ -82,9 +82,8 @@ void node_napi_env__::trigger_fatal_exception(v8::Local<v8::Value> local_err) {
   node::errors::TriggerUncaughtException(isolate, local_err, local_msg);
 }
 
-// option enforceUncaughtExceptionPolicy is added for not breaking existing
-// running n-api add-ons, and should be deprecated in the next major Node.js
-// release.
+// The option enforceUncaughtExceptionPolicy is added for not breaking existing
+// running Node-API add-ons.
 template <bool enforceUncaughtExceptionPolicy, typename T>
 void node_napi_env__::CallbackIntoModule(T&& call) {
   CallIntoModule(call, [](napi_env env_, v8::Local<v8::Value> local_err) {
@@ -93,19 +92,24 @@ void node_napi_env__::CallbackIntoModule(T&& call) {
       return;
     }
     node::Environment* node_env = env->node_env();
-    if (!node_env->options()->force_node_api_uncaught_exceptions_policy &&
+    // If the module api version is less than NAPI_VERSION_EXPERIMENTAL,
+    // and the option --force-node-api-uncaught-exceptions-policy is not
+    // specified, emit a warning about the uncaught exception instead of
+    // triggering uncaught exception event.
+    if (env->module_api_version < NAPI_VERSION_EXPERIMENTAL &&
+        !node_env->options()->force_node_api_uncaught_exceptions_policy &&
         !enforceUncaughtExceptionPolicy) {
       ProcessEmitDeprecationWarning(
           node_env,
           "Uncaught N-API callback exception detected, please run node "
-          "with option --force-node-api-uncaught-exceptions-policy=true"
+          "with option --force-node-api-uncaught-exceptions-policy=true "
           "to handle those exceptions properly.",
           "DEP0168");
       return;
     }
     // If there was an unhandled exception in the complete callback,
     // report it as a fatal exception. (There is no JavaScript on the
-    // callstack that can possibly handle it.)
+    // call stack that can possibly handle it.)
     env->trigger_fatal_exception(local_err);
   });
 }

diff --git a/graal-nodejs/test/js-native-api/test_reference/binding.gyp b/graal-nodejs/test/js-native-api/test_reference/binding.gyp
@@ -5,6 +5,12 @@
       "sources": [
         "test_reference.c"
       ]
+    },
+    {
+      "target_name": "test_finalizer",
+      "sources": [
+        "test_finalizer.c"
+      ]
     }
   ]
 }
diff --git a/graal-nodejs/test/js-native-api/test_reference/test_finalizer.c b/graal-nodejs/test/js-native-api/test_reference/test_finalizer.c
@@ -0,0 +1,71 @@
+#include <assert.h>
+#include <js_native_api.h>
+#include <stdlib.h>
+#include "../common.h"
+#include "../entry_point.h"
+
+static int test_value = 1;
+static int finalize_count = 0;
+
+static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
+  int* actual_value = data;
+  NODE_API_ASSERT_RETURN_VOID(
+      env,
+      actual_value == &test_value,
+      "The correct pointer was passed to the finalizer");
+
+  napi_ref finalizer_ref = (napi_ref)hint;
+  napi_value js_finalizer;
+  napi_value recv;
+  NODE_API_CALL_RETURN_VOID(
+      env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
+  NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
+  NODE_API_CALL_RETURN_VOID(
+      env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
+  NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
+}
+
+static napi_value CreateExternalWithJsFinalize(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, NULL, NULL));
+  NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
+  napi_value finalizer = args[0];
+  napi_valuetype finalizer_valuetype;
+  NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
+  NODE_API_ASSERT(env,
+                  finalizer_valuetype == napi_function,
+                  "Wrong type of first argument");
+  napi_ref finalizer_ref;
+  NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
+
+  napi_value result;
+  NODE_API_CALL(env,
+                napi_create_external(env,
+                                     &test_value,
+                                     FinalizeExternalCallJs,
+                                     finalizer_ref, /* finalize_hint */
+                                     &result));
+
+  finalize_count = 0;
+  return result;
+}
+
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
+  napi_property_descriptor descriptors[] = {
+      DECLARE_NODE_API_PROPERTY("createExternalWithJsFinalize",
+                                CreateExternalWithJsFinalize),
+  };
+
+  NODE_API_CALL(
+      env,
+      napi_define_properties(env,
+                             exports,
+                             sizeof(descriptors) / sizeof(*descriptors),
+                             descriptors));
+
+  return exports;
+}
+EXTERN_C_END
diff --git a/graal-nodejs/test/js-native-api/test_reference/test_finalizer.js b/graal-nodejs/test/js-native-api/test_reference/test_finalizer.js
@@ -2,7 +2,7 @@
 // Flags: --expose-gc --force-node-api-uncaught-exceptions-policy
 
 const common = require('../../common');
-const test_reference = require(`./build/${common.buildType}/test_reference`);
+const binding = require(`./build/${common.buildType}/test_finalizer`);
 const assert = require('assert');
 
 process.on('uncaughtException', common.mustCall((err) => {
@@ -11,7 +11,7 @@ process.on('uncaughtException', common.mustCall((err) => {
 
 (async function() {
   {
-    test_reference.createExternalWithJsFinalize(
+    binding.createExternalWithJsFinalize(
       common.mustCall(() => {
         throw new Error('finalizer error');
       }));

diff --git a/graal-nodejs/test/js-native-api/test_reference/test_reference.c b/graal-nodejs/test/js-native-api/test_reference/test_reference.c
@@ -22,20 +22,6 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
   finalize_count++;
 }
 
-static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
-  int *actual_value = data;
-  NODE_API_ASSERT_RETURN_VOID(env, actual_value == &test_value,
-      "The correct pointer was passed to the finalizer");
-
-  napi_ref finalizer_ref = (napi_ref)hint;
-  napi_value js_finalizer;
-  napi_value recv;
-  NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
-  NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
-  NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
-  NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
-}
-
 static napi_value CreateExternal(napi_env env, napi_callback_info info) {
   int* data = &test_value;
 
@@ -118,31 +104,6 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
   return result;
 }
 
-static napi_value
-CreateExternalWithJsFinalize(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, NULL, NULL));
-  NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
-  napi_value finalizer = args[0];
-  napi_valuetype finalizer_valuetype;
-  NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
-  NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
-  napi_ref finalizer_ref;
-  NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
-
-  napi_value result;
-  NODE_API_CALL(env,
-      napi_create_external(env,
-                           &test_value,
-                           FinalizeExternalCallJs,
-                           finalizer_ref, /* finalize_hint */
-                           &result));
-
-  finalize_count = 0;
-  return result;
-}
-
 static napi_value CheckExternal(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value arg;
@@ -263,24 +224,24 @@ static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info
 EXTERN_C_START
 napi_value Init(napi_env env, napi_value exports) {
   napi_property_descriptor descriptors[] = {
-    DECLARE_NODE_API_GETTER("finalizeCount", GetFinalizeCount),
-    DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal),
-    DECLARE_NODE_API_PROPERTY("createExternalWithFinalize",
-        CreateExternalWithFinalize),
-    DECLARE_NODE_API_PROPERTY("createExternalWithJsFinalize",
-        CreateExternalWithJsFinalize),
-    DECLARE_NODE_API_PROPERTY("checkExternal", CheckExternal),
-    DECLARE_NODE_API_PROPERTY("createReference", CreateReference),
-    DECLARE_NODE_API_PROPERTY("createSymbol", CreateSymbol),
-    DECLARE_NODE_API_PROPERTY("createSymbolFor", CreateSymbolFor),
-    DECLARE_NODE_API_PROPERTY("createSymbolForEmptyString", CreateSymbolForEmptyString),
-    DECLARE_NODE_API_PROPERTY("createSymbolForIncorrectLength", CreateSymbolForIncorrectLength),
-    DECLARE_NODE_API_PROPERTY("deleteReference", DeleteReference),
-    DECLARE_NODE_API_PROPERTY("incrementRefcount", IncrementRefcount),
-    DECLARE_NODE_API_PROPERTY("decrementRefcount", DecrementRefcount),
-    DECLARE_NODE_API_GETTER("referenceValue", GetReferenceValue),
-    DECLARE_NODE_API_PROPERTY("validateDeleteBeforeFinalize",
-                          ValidateDeleteBeforeFinalize),
+      DECLARE_NODE_API_GETTER("finalizeCount", GetFinalizeCount),
+      DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal),
+      DECLARE_NODE_API_PROPERTY("createExternalWithFinalize",
+                                CreateExternalWithFinalize),
+      DECLARE_NODE_API_PROPERTY("checkExternal", CheckExternal),
+      DECLARE_NODE_API_PROPERTY("createReference", CreateReference),
+      DECLARE_NODE_API_PROPERTY("createSymbol", CreateSymbol),
+      DECLARE_NODE_API_PROPERTY("createSymbolFor", CreateSymbolFor),
+      DECLARE_NODE_API_PROPERTY("createSymbolForEmptyString",
+                                CreateSymbolForEmptyString),
+      DECLARE_NODE_API_PROPERTY("createSymbolForIncorrectLength",
+                                CreateSymbolForIncorrectLength),
+      DECLARE_NODE_API_PROPERTY("deleteReference", DeleteReference),
+      DECLARE_NODE_API_PROPERTY("incrementRefcount", IncrementRefcount),
+      DECLARE_NODE_API_PROPERTY("decrementRefcount", DecrementRefcount),
+      DECLARE_NODE_API_GETTER("referenceValue", GetReferenceValue),
+      DECLARE_NODE_API_PROPERTY("validateDeleteBeforeFinalize",
+                                ValidateDeleteBeforeFinalize),
   };
 
   NODE_API_CALL(env, napi_define_properties(

diff --git a/graal-nodejs/test/node-api/test_buffer/binding.gyp b/graal-nodejs/test/node-api/test_buffer/binding.gyp
@@ -3,6 +3,10 @@
     {
       "target_name": "test_buffer",
       "sources": [ "test_buffer.c" ]
+    },
+    {
+      "target_name": "test_finalizer",
+      "sources": [ "test_finalizer.c" ]
     }
   ]
 }
diff --git a/graal-nodejs/test/node-api/test_buffer/test_buffer.c b/graal-nodejs/test/node-api/test_buffer/test_buffer.c
@@ -22,17 +22,6 @@ static void noopDeleter(napi_env env, void* data, void* finalize_hint) {
   deleterCallCount++;
 }
 
-static void malignDeleter(napi_env env, void* data, void* finalize_hint) {
-  NODE_API_ASSERT_RETURN_VOID(env, data != NULL && strcmp(data, theText) == 0, "invalid data");
-  napi_ref finalizer_ref = (napi_ref)finalize_hint;
-  napi_value js_finalizer;
-  napi_value recv;
-  NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
-  NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
-  NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
-  NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
-}
-
 static napi_value newBuffer(napi_env env, napi_callback_info info) {
   napi_value theBuffer;
   char* theCopy;
@@ -118,30 +107,6 @@ static napi_value staticBuffer(napi_env env, napi_callback_info info) {
   return theBuffer;
 }
 
-static napi_value malignFinalizerBuffer(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, NULL, NULL));
-  NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
-  napi_value finalizer = args[0];
-  napi_valuetype finalizer_valuetype;
-  NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
-  NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
-  napi_ref finalizer_ref;
-  NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
-
-  napi_value theBuffer;
-  NODE_API_CALL(
-      env,
-      napi_create_external_buffer(env,
-                                  sizeof(theText),
-                                  (void*)theText,
-                                  malignDeleter,
-                                  finalizer_ref,  // finalize_hint
-                                  &theBuffer));
-  return theBuffer;
-}
-
 static napi_value Init(napi_env env, napi_value exports) {
   napi_value theValue;
 
@@ -151,14 +116,13 @@ static napi_value Init(napi_env env, napi_value exports) {
       napi_set_named_property(env, exports, "theText", theValue));
 
   napi_property_descriptor methods[] = {
-    DECLARE_NODE_API_PROPERTY("newBuffer", newBuffer),
-    DECLARE_NODE_API_PROPERTY("newExternalBuffer", newExternalBuffer),
-    DECLARE_NODE_API_PROPERTY("getDeleterCallCount", getDeleterCallCount),
-    DECLARE_NODE_API_PROPERTY("copyBuffer", copyBuffer),
-    DECLARE_NODE_API_PROPERTY("bufferHasInstance", bufferHasInstance),
-    DECLARE_NODE_API_PROPERTY("bufferInfo", bufferInfo),
-    DECLARE_NODE_API_PROPERTY("staticBuffer", staticBuffer),
-    DECLARE_NODE_API_PROPERTY("malignFinalizerBuffer", malignFinalizerBuffer),
+      DECLARE_NODE_API_PROPERTY("newBuffer", newBuffer),
+      DECLARE_NODE_API_PROPERTY("newExternalBuffer", newExternalBuffer),
+      DECLARE_NODE_API_PROPERTY("getDeleterCallCount", getDeleterCallCount),
+      DECLARE_NODE_API_PROPERTY("copyBuffer", copyBuffer),
+      DECLARE_NODE_API_PROPERTY("bufferHasInstance", bufferHasInstance),
+      DECLARE_NODE_API_PROPERTY("bufferInfo", bufferInfo),
+      DECLARE_NODE_API_PROPERTY("staticBuffer", staticBuffer),
   };
 
   NODE_API_CALL(env, napi_define_properties(