-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for requiring basic finalizers #1568
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). To ensure that all finalizers are basic | ||
finalizers at compile-time, define the `NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering if we should add a "best practice" target that enables all possible checks in https://github.com/nodejs/node-addon-api/blob/main/node_addon_api.gyp. |
||
preprocessor directive. | ||
|
||
## Finalizers | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,3 +90,13 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too: "The ... can be defined in order to bypass ... " There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 23b57cc |
||
preprocessor directive `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS`. | ||
|
||
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. To | ||
disable the convenience helpers, define the preprocessor directive | ||
`NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS`. | ||
|
||
[_finalizers_]: ./finalization.md |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,14 +205,19 @@ struct FinalizeData { | |
}); | ||
} | ||
|
||
#ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | ||
#if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, this was a leftover of a different implementation. Addressed in 23b57cc |
||
template <typename F = Finalizer, | ||
typename = std::enable_if_t< | ||
!std::is_invocable_v<F, node_api_nogc_env, T*>>, | ||
typename = void> | ||
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( | ||
|
@@ -235,14 +240,19 @@ struct FinalizeData { | |
}); | ||
} | ||
|
||
#ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | ||
#if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 23b57cc |
||
template <typename F = Finalizer, | ||
typename = std::enable_if_t< | ||
!std::is_invocable_v<F, node_api_nogc_env, T*, Hint*>>, | ||
typename = void> | ||
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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)'); | ||
} | ||
})(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
package-lock=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
#include <napi.h> | ||
|
||
Napi::Object Init(Napi::Env env, Napi::Object exports) { | ||
exports.Set( | ||
"external", | ||
Napi::External<int>::New( | ||
env, new int(1), [](Napi::Env /*env*/, int* data) { delete data; })); | ||
|
||
return exports; | ||
} | ||
|
||
NODE_API_MODULE(NODE_GYP_MODULE_NAME, Init) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
{ | ||
'target_defaults': { | ||
'include_dirs': [ | ||
"<!(node -p \"require('node-addon-api').include_dir\")" | ||
], | ||
'variables': { | ||
'NAPI_VERSION%': "<!(node -p \"process.env.NAPI_VERSION || process.versions.napi\")", | ||
'require_basic_finalizers': "<!(node -p \"process.env['npm_config_require_basic_finalizers']\")", | ||
}, | ||
'conditions': [ | ||
['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ], | ||
['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ], | ||
['require_basic_finalizers=="true"', { | ||
'defines': ['NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS'], | ||
}], | ||
['OS=="mac"', { | ||
'cflags+': ['-fvisibility=hidden'], | ||
'xcode_settings': { | ||
'OTHER_CFLAGS': ['-fvisibility=hidden'] | ||
} | ||
}] | ||
], | ||
'sources': [ | ||
'addon.cc', | ||
], | ||
}, | ||
'targets': [ | ||
{ | ||
'target_name': 'addon', | ||
'defines': [ | ||
'NAPI_CPP_EXCEPTIONS' | ||
], | ||
'cflags!': [ '-fno-exceptions' ], | ||
'cflags_cc!': [ '-fno-exceptions' ], | ||
'msvs_settings': { | ||
'VCCLCompilerTool': { | ||
'ExceptionHandling': 1, | ||
'EnablePREfast': 'true', | ||
}, | ||
}, | ||
'xcode_settings': { | ||
'CLANG_CXX_LIBRARY': 'libc++', | ||
'MACOSX_DEPLOYMENT_TARGET': '10.7', | ||
'GCC_ENABLE_CPP_EXCEPTIONS': 'YES', | ||
}, | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
'use strict'; | ||
|
||
module.exports = require('bindings')('addon'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"name": "addon", | ||
"version": "0.0.0", | ||
"description": "Node.js addon", | ||
"private": true, | ||
"scripts": {}, | ||
"dependencies": { | ||
"node-addon-api": "file:../../../../" | ||
}, | ||
"gypfile": true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this passive voice: The
NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS
preprocessor directive can be defined to ensure that all finalizers are basic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 23b57cc