From 7bcb826aa4323f450b3c58f9c7fb34243ff13f77 Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Thu, 19 Sep 2024 18:01:46 +0200 Subject: [PATCH] feat: add support for requiring basic finalizers (#1568) * feat: add support for requiring basic finalizers * Address review comments - Use passive voice in existing and new docs - Revert unnecessary change --- .gitignore | 1 + doc/finalization.md | 4 +- doc/setup.md | 21 ++++++-- napi-inl.h | 10 ++++ test/require_basic_finalizers/index.js | 38 +++++++++++++++ test/require_basic_finalizers/tpl/.npmrc | 1 + test/require_basic_finalizers/tpl/addon.cc | 12 +++++ test/require_basic_finalizers/tpl/binding.gyp | 48 +++++++++++++++++++ test/require_basic_finalizers/tpl/index.js | 3 ++ .../require_basic_finalizers/tpl/package.json | 11 +++++ 10 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 test/require_basic_finalizers/index.js create mode 100644 test/require_basic_finalizers/tpl/.npmrc create mode 100644 test/require_basic_finalizers/tpl/addon.cc create mode 100644 test/require_basic_finalizers/tpl/binding.gyp create mode 100644 test/require_basic_finalizers/tpl/index.js create mode 100644 test/require_basic_finalizers/tpl/package.json diff --git a/.gitignore b/.gitignore index f6b80ae92..a154db646 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ /benchmark/build /benchmark/src /test/addon_build/addons +/test/require_basic_finalizers/addons /.vscode # ignore node-gyp generated files outside its build directory diff --git a/doc/finalization.md b/doc/finalization.md index 825ff742a..3dc4e7860 100644 --- a/doc/finalization.md +++ b/doc/finalization.md @@ -8,7 +8,9 @@ provide more efficient memory management, optimizations, improved execution, or other benefits. In general, it is best to use basic finalizers whenever possible (eg. when -access to JavaScript is _not_ needed). +access to JavaScript is _not_ needed). The +`NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` preprocessor directive can be defined +to ensure that all finalizers are basic. ## Finalizers diff --git a/doc/setup.md b/doc/setup.md index 29bb1a743..86a1b0f38 100644 --- a/doc/setup.md +++ b/doc/setup.md @@ -81,12 +81,23 @@ To use **Node-API** in a native module: At build time, the Node-API back-compat library code will be used only when the targeted node version *does not* have Node-API built-in. -The preprocessor directive `NODE_ADDON_API_DISABLE_DEPRECATED` can be defined at -compile time before including `napi.h` to skip the definition of deprecated APIs. +The `NODE_ADDON_API_DISABLE_DEPRECATED` preprocessor directive can be defined at +compile time before including `napi.h` to skip the definition of deprecated +APIs. By default, throwing an exception on a terminating environment (eg. worker threads) will cause a fatal exception, terminating the Node process. This is to provide feedback to the user of the runtime error, as it is impossible to pass -the error to JavaScript when the environment is terminating. In order to bypass -this behavior such that the Node process will not terminate, define the -preprocessor directive `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS`. +the error to JavaScript when the environment is terminating. The +`NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` preprocessor directive can be defined +to bypass this behavior, such that the Node process will not terminate. + +Various Node-API constructs provide a mechanism to run a callback in response to +a garbage collection event of that object. These callbacks are called +[_finalizers_]. Some finalizers have restrictions on the type of Node-APIs +available within the callback. node-addon-api provides convenience helpers that +bypass this limitation, but may cause the add-on to run less efficiently. The +`NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` preprocessor directive can be defined +to disable the convenience helpers. + +[_finalizers_]: ./finalization.md diff --git a/napi-inl.h b/napi-inl.h index c0f711b68..e7e21a7b3 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -213,6 +213,11 @@ struct FinalizeData { static inline void Wrapper(node_api_nogc_env env, void* data, void* finalizeHint) NAPI_NOEXCEPT { +#ifdef NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS + static_assert(false, + "NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS defined: Finalizer " + "must be basic."); +#endif napi_status status = node_api_post_finalizer(env, WrapperGC, data, finalizeHint); NAPI_FATAL_IF_FAILED( @@ -243,6 +248,11 @@ struct FinalizeData { static inline void WrapperWithHint(node_api_nogc_env env, void* data, void* finalizeHint) NAPI_NOEXCEPT { +#ifdef NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS + static_assert(false, + "NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS defined: Finalizer " + "must be basic."); +#endif napi_status status = node_api_post_finalizer(env, WrapperGCWithHint, data, finalizeHint); NAPI_FATAL_IF_FAILED( diff --git a/test/require_basic_finalizers/index.js b/test/require_basic_finalizers/index.js new file mode 100644 index 000000000..31ee4f00b --- /dev/null +++ b/test/require_basic_finalizers/index.js @@ -0,0 +1,38 @@ +'use strict'; + +const { promisify } = require('util'); +const exec = promisify(require('child_process').exec); +const { copy, remove } = require('fs-extra'); +const path = require('path'); +const assert = require('assert'); + +async function test () { + const addon = 'require-basic-finalizers'; + const ADDON_FOLDER = path.join(__dirname, 'addons', addon); + + await remove(ADDON_FOLDER); + await copy(path.join(__dirname, 'tpl'), ADDON_FOLDER); + + console.log(' >Building addon'); + + // Fail when NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS is enabled + await assert.rejects(exec('npm --require-basic-finalizers install', { + cwd: ADDON_FOLDER + }), 'Addon unexpectedly compiled successfully'); + + // Succeed when NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS is not enabled + return assert.doesNotReject(exec('npm install', { + cwd: ADDON_FOLDER + })); +} + +module.exports = (function () { + // This test will only run under an experimental version test. + const isExperimental = Number(process.env.NAPI_VERSION) === 2147483647; + + if (isExperimental) { + return test(); + } else { + console.log(' >Skipped (non-experimental test run)'); + } +})(); diff --git a/test/require_basic_finalizers/tpl/.npmrc b/test/require_basic_finalizers/tpl/.npmrc new file mode 100644 index 000000000..43c97e719 --- /dev/null +++ b/test/require_basic_finalizers/tpl/.npmrc @@ -0,0 +1 @@ +package-lock=false diff --git a/test/require_basic_finalizers/tpl/addon.cc b/test/require_basic_finalizers/tpl/addon.cc new file mode 100644 index 000000000..f4277ac74 --- /dev/null +++ b/test/require_basic_finalizers/tpl/addon.cc @@ -0,0 +1,12 @@ +#include + +Napi::Object Init(Napi::Env env, Napi::Object exports) { + exports.Set( + "external", + Napi::External::New( + env, new int(1), [](Napi::Env /*env*/, int* data) { delete data; })); + + return exports; +} + +NODE_API_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/require_basic_finalizers/tpl/binding.gyp b/test/require_basic_finalizers/tpl/binding.gyp new file mode 100644 index 000000000..caf99d21f --- /dev/null +++ b/test/require_basic_finalizers/tpl/binding.gyp @@ -0,0 +1,48 @@ +{ + 'target_defaults': { + 'include_dirs': [ + "