FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

node-api: document node-api shutdown finalization by legendecas · Pull Request #45903 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
legendecas:node-api/shutdown
Mar 20, 2023
Merged

node-api: document node-api shutdown finalization#45903
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
legendecas:node-api/shutdown

Conversation

legendecas commented Dec 18, 2022
edited
Loading

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/node-api

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
Comment thread doc/api/n-api.md
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you mean these run in parallel?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, their napi_finalize callbacks can be run in an interleaving pattern, on the JavaScript thread.

Comment thread doc/api/n-api.md
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 20, 2023
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 merged commit 2984cc3 into nodejs:main Mar 20, 2023

Copy link
Copy Markdown
Collaborator

Landed in 2984cc3

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 <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
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 <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
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 <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL