node-api: document node-api shutdown finalization #45903
Conversation
As status quo, the cleanup hooks are invoked before the `napi_finalize` callbacks at the exit of Node.js environments. This gives addons a chance to release their resource in a proper order manually. Document this behavior explicitly to advocate the usage on cleanup hooks instead of relying on the implied invocation of `napi_finalize` callbacks at shutdown.
|
Review requested:
|
nodejs-github-bot
added
doc
| possible with JavaScript execution disallowed, like on the request of | ||
| [`worker.terminate()`][]. When the environment is being torn down, the | ||
| registered `napi_finalize` callbacks of JavaScript objects, Thread-safe | ||
| functions and environment instance data are invoked immediately and |
There was a problem hiding this comment.
Do you mean these run in parallel?
There was a problem hiding this comment.
No, their napi_finalize callbacks can be run in an interleaving pattern, on the JavaScript thread.
| registered cleanup hooks. In order to ensure a proper order of addon | ||
| finalization during environment shutdown to avoid use-after-free in the | ||
| `napi_finalize` callback, addons should register a cleanup hook with | ||
| `napi_add_env_cleanup_hook` and `napi_add_async_cleanup_hook` to manually |
There was a problem hiding this comment.
Can you expand a bit here? Is the suggestion to use the hooks instead of napi_finalize callback?
There was a problem hiding this comment.
This part is specifically focused on the finalization at shutdown. Still, addons should set an napi_finalize callback to finalize the native objects properly.
However, their napi_finalize callbacks can be run in an interleaving pattern forcefully, regardless of if they are actually referenced. This would lead to the problem mentioned in nodejs/node-addon-api#1109:
Normally, we expect the order to be:
- create an ObjectWrap,
- create a ThreadSafeFunction and save it to a field of that ObjectWrap,
- release the ObjectWrap,
- release the ThreadSafeFunction in the
~ObjectWrap.
However, when the environment is shutting down, the order would be:
- create an ObjectWrap,
- create a ThreadSafeFunction and save it to a field of that ObjectWrap,
- env shutdown, run
napi_finalizein reversed creation order, - release the ThreadSafeFunction,
- release the ObjectWrap => crash as the ThreadSafeFunction has already been finalized.
There was a problem hiding this comment.
@legendecas Does this mean that I am supposed to maintain a central directory of all ThreadSafeFunction by environment so that I can Release in the cleanup hook because by the time the ObjectWrap destructor is called, it would be too late?
There was a problem hiding this comment.
@legendecas in particular, if I use napi_add_async_cleanup_hook, can I delay the destruction of the environment until all ObjectWraps has been destroyed? Ie, will the destructors get called while Node is waiting for napi_remove_async_cleanup_hook? I strongly suggest that you do at least this, because otherwise this would mean that all ObjectWraps are to have a two-staged destruction with a C++ destructor and a, lets call it ::Destroy(), method which can be called both from the destructor and the environment cleanup hook. This would throw out of the window all the work done on the clean node-addon-api interface which aligns the JS object lifecycle with the C++ constructor/destructor.
There was a problem hiding this comment.
@mmomtchev Does this mean that I am supposed to maintain a central directory of all ThreadSafeFunction by environment so that I can Release in the cleanup hook because by the time the ObjectWrap destructor is called, it would be too late?
If the ThreadSafeFunction is saved as a field of a c++ ObjectWrap, it should be released in the destructor of the ObjectWrap. So it is sufficient for an add-on to keep track of ObjectWraps and released them in a napi_cleanup_hook.
in particular, if I use napi_add_async_cleanup_hook, can I delay the destruction of the environment until all ObjectWraps has been destroyed? Ie, will the destructors get called while Node is waiting for napi_remove_async_cleanup_hook?
This is how napi_async_cleanup_hook_handle works now. If an napi_async_cleanup_hook_handle is still pending removal by napi_remove_async_cleanup_hook, the napi_env is not going to be destructed. This is verified with tests at #46692.
legendecas
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 2984cc3 |
As status quo, the cleanup hooks are invoked before the `napi_finalize` callbacks at the exit of Node.js environments. This gives addons a chance to release their resource in a proper order manually. Document this behavior explicitly to advocate the usage on cleanup hooks instead of relying on the implied invocation of `napi_finalize` callbacks at shutdown. PR-URL: #45903 Fixes: #45088 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
As status quo, the cleanup hooks are invoked before the `napi_finalize` callbacks at the exit of Node.js environments. This gives addons a chance to release their resource in a proper order manually. Document this behavior explicitly to advocate the usage on cleanup hooks instead of relying on the implied invocation of `napi_finalize` callbacks at shutdown. PR-URL: #45903 Fixes: #45088 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
As status quo, the cleanup hooks are invoked before the `napi_finalize` callbacks at the exit of Node.js environments. This gives addons a chance to release their resource in a proper order manually. Document this behavior explicitly to advocate the usage on cleanup hooks instead of relying on the implied invocation of `napi_finalize` callbacks at shutdown. PR-URL: #45903 Fixes: #45088 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
As status quo, the cleanup hooks are invoked before the
napi_finalizecallbacks at the exit of Node.js environments. This gives addons a
chance to release their resource in a proper order manually.
Document this behavior explicitly to advocate the usage on cleanup
hooks instead of relying on the implied invocation of
napi_finalizecallbacks at shutdown.
This PR also unifies the term "Node.js instance" and "Agent" as "Node.js environment".
Fixes: #45088