feat: add support for requiring basic finalizers #1568
Conversation
KevinEady
force-pushed
the
add-require-basic-finalizers
branch
from
4603869 to
e21c0f6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1568 +/- ##
=======================================
Coverage 64.40% 64.40%
=======================================
Files 3 3
Lines 2003 2003
Branches 693 693
=======================================
Hits 1290 1290
Misses 146 146
Partials 567 567 ☔ View full report in Codecov by Sentry. |
KevinEady
force-pushed
the
add-require-basic-finalizers
branch
from
e21c0f6 to
7c91a10
Compare
|
When trying to use a non-basic finalizer with this directive defined, you'll get an error like: (error taken from the test created in this PR) |
doc/finalization.md
Outdated
| 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.
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.
doc/finalization.md
Outdated
|
|
||
| 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 |
There was a problem hiding this comment.
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.
Addressed in 23b57cc
doc/setup.md
Outdated
| 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 |
There was a problem hiding this comment.
Here too: "The ... can be defined in order to bypass ... "
There was a problem hiding this comment.
Addressed in 23b57cc
napi-inl.h
Outdated
| } | ||
|
|
||
| #ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | ||
| #if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) |
There was a problem hiding this comment.
Is this change needed?
There was a problem hiding this comment.
Whoops, this was a leftover of a different implementation. Addressed in 23b57cc
napi-inl.h
Outdated
| } | ||
|
|
||
| #ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | ||
| #if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) |
There was a problem hiding this comment.
Addressed in 23b57cc
- Use passive voice in existing and new docs - Revert unnecessary change
Hi @gabrielschulhof , Your changes have been introduced in 23b57cc. PTAL! |
Introduce
NODE_ADDON_API_REQUIRE_BASIC_FINALIZERSpreprocessor directive, which adds an always-fail static assertion inside theWrapperdefinition (responsible for passing the user's non-basic finalizer through anode_api_post_finalizercall) if this preprocessor directive is defined.