diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 002f8be0a568f7..ba7b4df23fc2b6 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5288,6 +5288,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` + + + +```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 diff --git a/src/js_native_api.h b/src/js_native_api.h index 364d3672d1c344..bf4e73d37ee7e0 100644 --- a/src/js_native_api.h +++ b/src/js_native_api.h @@ -562,6 +562,12 @@ NAPI_EXTERN napi_status napi_object_freeze(napi_env env, napi_value object); 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_ diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 611a0521fceab8..55a24c4300cae8 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -57,6 +57,54 @@ (out) = v8::type::New((buffer), (byte_offset), (length)); \ } while (0) +void napi_env__::HandleFinalizerException(napi_env env, + v8::Local 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 v8handler = + v8::Local::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 { @@ -3210,3 +3258,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 handler = + v8impl::V8LocalValueFromJsValue(error_handler); + if (handler->IsFunction()) { + env->finalizer_error_handler = + v8impl::Persistent(env->isolate, handler); + } else { + env->finalizer_error_handler.Reset(); + } + } else { + env->finalizer_error_handler.Reset(); + } + + return napi_clear_last_error(env); +} diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index ffe351f4f6c65e..8bc30b56e4fa37 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -102,9 +102,13 @@ struct napi_env__ { } } + static void HandleFinalizerException(napi_env env, + v8::Local 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 last_exception; @@ -119,6 +123,7 @@ struct napi_env__ { int open_callback_scopes = 0; int refs = 1; void* instance_data = nullptr; + v8impl::Persistent finalizer_error_handler; }; // This class is used to keep a napi_env live in a way that diff --git a/src/node_api.cc b/src/node_api.cc index aa4f8a6a2401e2..525e0176f7ca9a 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -35,17 +35,18 @@ v8::Maybe 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(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 { diff --git a/test/js-native-api/test_finalizer_exception/binding.gyp b/test/js-native-api/test_finalizer_exception/binding.gyp new file mode 100644 index 00000000000000..71a67d4a809699 --- /dev/null +++ b/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" + ] + } + ] +} diff --git a/test/js-native-api/test_finalizer_exception/test_finalizer_exception.cc b/test/js-native-api/test_finalizer_exception/test_finalizer_exception.cc new file mode 100644 index 00000000000000..225d5469b19034 --- /dev/null +++ b/test/js-native-api/test_finalizer_exception/test_finalizer_exception.cc @@ -0,0 +1,51 @@ +#define NAPI_EXPERIMENTAL +#include +#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 diff --git a/test/js-native-api/test_finalizer_exception/test_handled_exception.js b/test/js-native-api/test_finalizer_exception/test_handled_exception.js new file mode 100644 index 00000000000000..1b882fa5e5c7b9 --- /dev/null +++ b/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(); diff --git a/test/js-native-api/test_finalizer_exception/test_no_exception.js b/test/js-native-api/test_finalizer_exception/test_no_exception.js new file mode 100644 index 00000000000000..d61e9dd81934e6 --- /dev/null +++ b/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(); diff --git a/test/js-native-api/test_finalizer_exception/test_unhandled_exception.js b/test/js-native-api/test_finalizer_exception/test_unhandled_exception.js new file mode 100644 index 00000000000000..0900085882591b --- /dev/null +++ b/test/js-native-api/test_finalizer_exception/test_unhandled_exception.js @@ -0,0 +1,44 @@ +'use strict'; +// Flags: --expose-gc + +// The test verifies that if finalizer throws an exception, +// then it is 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'); + }); + + (() => { + test.createObject(true /* throw on destruct */); + })(); + gcUntilSync(1); + assert.strictEqual(test.finalizeCount, 1); + assert.strictEqual(unhandledExceptions, 1); + + (() => { + test.createObject(true /* throw on destruct */); + test.createObject(true /* throw on destruct */); + })(); + gcUntilSync(3); + assert.strictEqual(test.finalizeCount, 3); + assert.strictEqual(unhandledExceptions, 3); +} +runGCTests(); diff --git a/test/js-native-api/test_finalizer_exception/testobject.cc b/test/js-native-api/test_finalizer_exception/testobject.cc new file mode 100644 index 00000000000000..ff4db55f8aa6d1 --- /dev/null +++ b/test/js-native-api/test_finalizer_exception/testobject.cc @@ -0,0 +1,94 @@ +#include "testobject.h" +#include "../common.h" + +static int finalize_count = 0; + +TestObject::TestObject(bool throw_js_in_destructor) + : env_(nullptr), + wrapper_(nullptr), + throw_js_in_destructor_(throw_js_in_destructor) {} + +TestObject::~TestObject() { + napi_delete_reference(env_, wrapper_); +} + +void TestObject::Destructor(napi_env env, + void* nativeObject, + void* /*finalize_hint*/) { + ++finalize_count; + TestObject* obj = static_cast(nativeObject); + bool throw_js_in_destructor = obj->throw_js_in_destructor_; + delete obj; + if (throw_js_in_destructor) { + NODE_API_CALL_RETURN_VOID( + env, napi_throw_error(env, nullptr, "Error during Finalize")); + } +} + +napi_value TestObject::GetFinalizeCount(napi_env env, napi_callback_info info) { + napi_value result; + NODE_API_CALL(env, napi_create_int32(env, finalize_count, &result)); + return result; +} + +napi_ref TestObject::constructor; + +napi_status TestObject::Init(napi_env env) { + napi_status status; + napi_value cons; + status = + napi_define_class(env, "TestObject", -1, New, nullptr, 0, nullptr, &cons); + if (status != napi_ok) return status; + + status = napi_create_reference(env, cons, 1, &constructor); + if (status != napi_ok) return status; + + return napi_ok; +} + +napi_value TestObject::New(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value args[1]; + napi_value _this; + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, &_this, nullptr)); + + napi_valuetype valuetype; + NODE_API_CALL(env, napi_typeof(env, args[0], &valuetype)); + + bool throw_js_in_destructor{false}; + if (valuetype == napi_boolean) { + NODE_API_CALL(env, + napi_get_value_bool(env, args[0], &throw_js_in_destructor)); + } + + TestObject* obj = new TestObject(throw_js_in_destructor); + + obj->env_ = env; + NODE_API_CALL(env, + napi_wrap(env, + _this, + obj, + TestObject::Destructor, + nullptr /* finalize_hint */, + &obj->wrapper_)); + + return _this; +} + +napi_status TestObject::NewInstance(napi_env env, + napi_value arg, + napi_value* instance) { + napi_status status; + + const int argc = 1; + napi_value argv[argc] = {arg}; + + napi_value cons; + status = napi_get_reference_value(env, constructor, &cons); + if (status != napi_ok) return status; + + status = napi_new_instance(env, cons, argc, argv, instance); + if (status != napi_ok) return status; + + return napi_ok; +} diff --git a/test/js-native-api/test_finalizer_exception/testobject.h b/test/js-native-api/test_finalizer_exception/testobject.h new file mode 100644 index 00000000000000..d622f896e416ad --- /dev/null +++ b/test/js-native-api/test_finalizer_exception/testobject.h @@ -0,0 +1,28 @@ +#ifndef TEST_JS_NATIVE_API_TEST_FINALIZER_EXCEPTION_TESTOBJECT_H_ +#define TEST_JS_NATIVE_API_TEST_FINALIZER_EXCEPTION_TESTOBJECT_H_ + +#include + +class TestObject { + public: + static napi_status Init(napi_env env); + static void Destructor(napi_env env, void* nativeObject, void* finalize_hint); + static napi_value GetFinalizeCount(napi_env env, napi_callback_info info); + static napi_status NewInstance(napi_env env, + napi_value arg, + napi_value* instance); + + private: + explicit TestObject(bool throw_js_in_destructor); + ~TestObject(); + + static napi_value New(napi_env env, napi_callback_info info); + + private: + static napi_ref constructor; + napi_env env_; + napi_ref wrapper_; + bool throw_js_in_destructor_{false}; +}; + +#endif // TEST_JS_NATIVE_API_TEST_FINALIZER_EXCEPTION_TESTOBJECT_H_