Skip to content
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

node-api: document node-api shutdown finalization #45903

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 18, 2022

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.

This PR also unifies the term "Node.js instance" and "Agent" as "Node.js environment".

Fixes: #45088

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.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Dec 18, 2022
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean these run in parallel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand a bit here? Is the suggestion to use the hooks instead of napi_finalize callback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. create an ObjectWrap,
  2. create a ThreadSafeFunction and save it to a field of that ObjectWrap,
  3. release the ObjectWrap,
  4. release the ThreadSafeFunction in the ~ObjectWrap.

However, when the environment is shutting down, the order would be:

  1. create an ObjectWrap,
  2. create a ThreadSafeFunction and save it to a field of that ObjectWrap,
  3. env shutdown, run napi_finalize in reversed creation order,
  4. release the ThreadSafeFunction,
  5. release the ObjectWrap => crash as the ThreadSafeFunction has already been finalized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2984cc3 into nodejs:main Mar 20, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2984cc3

@legendecas legendecas deleted the node-api/shutdown branch March 20, 2023 09:59
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectWrap destructors run after CleanupHook
5 participants