n-api: re-implement async env cleanup hooks by gabrielschulhof · Pull Request #34819 · nodejs/node · GitHub

Avoid passing core void* and function pointers into add-on.
Document napi_async_cleanup_hook_handle type.
Render receipt of the handle mandatory. Removal of the handle remains
mandatory.

Fixes: #34715
Signed-off-by: Gabriel Schulhof gabriel.schulhof@intel.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Review requested:

@nodejs/n-api

I removed the need to pass napi_env here because we wish to discourage JS execution in environment cleanup hooks and this way it should prevent add-ons from having to store the env for the sole purpose of calling this function from a uv_close callback.

We store napi_env internally and it's guaranteed to be usable for the lifetime of a remove_handle, because we Ref() it during its construction and Unref() it asynchronously during its destruction.

As mentioned in the issue, I'm missing the motivation for this change here.

I, personally, feel like the previous interface was a bit clearer because the signature of the cleanup hook implied that the callback function needed to be called.

In any case, the documentation needs to be updated with changes: sections.

If you're removing the signature here, please add a link to the definition of napi_async_cleanup_hook

Added the link below.

I do like how this better hides the internal implementation. It seemed like we were leaking a bit more than needed in the N-API APIs wrapping the internal node api.

@addaleax I have addressed your review comments.

Qualitatively, I think this approach makes napi_add_async_cleanup_hook() and napi_remove_async_cleanup_hook more into bookends for a specific cleanup operation. Furthermore, there is a nice symmetry produced by the fact that the entirety of the setup for a hook happens in its constructor, and the entirety of its teardown happens in its destructor. Additionally, it leaves only one value for the add-on maintainer to track.

I understand that these are all qualitative arguments and, as such, more or less a matter of taste. Quantitatively, perhaps, we can look at the test and see that it is simplified by 18 lines, though, since the change removes support for optional storage of the handle, the six removed lines that tested for that feature should perhaps not be considered. That still leaves 12 lines that are an indication that perhaps using async cleanup hooks by way of this API rather than the previous API results in less add-on code that needs writing and maintaining.

@gabrielschulhof I mean, I think any high-level wrapper like the C++ one would expose these two versions the same way anyway :)

Ultimately, I'm good with this if you think it's worth the breakage.

This is an overview for those who might wish to review of the current implementation vs. the proposed implementation showing how each would handle the most common use case:

Current implementation                               Proposed implementation
|                                                    |
v                                                    v
+------------------------------------------------+   +----------------------------------------------+
| Create async thing (e.g. uv handle)            |   | Create async thing (e.g. uv handle)          |
| napi_add_async_cleanup_hook(hook_cb)           |   | napi_add_async_cleanup_hook(hook_cb)         |
| Optional: Store returned hook_handle           | ! | Mandatory: Store returned hook_handle        |
+------------------------------------------------+   +----------------------------------------------+
|                                                    |
v                                                    v
+- hook_cb: -------------------------------------+   +- hook_cb: -----------------------------------+
| store done_cb (function ptr)                   | ! |                                              |
| store done_arg (void*)                         | ! |                                              |
| if a handle was stored                         | ! |                                              |
|   call napi_remove_async_cleanup_hook(handle)  | ! |                                              |
| Start closing the async thing (e.g uv_close)   |   | Start closing the async thing (e.g uv_close) |
+------------------------------------------------+   +----------------------------------------------+
|                                                    |
v                                                    v
+- uv_close cb: ---------------------------------+   +- uv_close cb: -------------------------------+
| call done_cb(done_arg)                         | ! | call napi_remove_async_cleanup_hook(handle)  |
+------------------------------------------------+   +----------------------------------------------+


+--+
|  | <-- an iteration of the event loop
+--+

* Avoid passing core `void*` and function pointers into add-on. * Document `napi_async_cleanup_hook_handle` type. * Render receipt of the handle mandatory. Removal of the handle remains mandatory. Fixes: nodejs#34715 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

Co-authored-by: Anna Henningsen <github@addaleax.net>

@addaleax I added the changes: sections you requested.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/32941/

I think it can still be optional to get the handle when the hook is added. When the hook is called it is passed the handle so unless it wants to call remove before the hook runs, it does not need to get/store the handle. I'd suggest we change back to this being optional.

LGTM after being updated to make getting the handle on additional optional like it was before.

The assumption is that there should be little to no usage of the existing API as it's only be out for ~ 3weeks from what Gabriel tells me and as an Experimental API would should be able to change it in this case.

@mhdawson it is once more optional to receive and store the handle during creation. It is nevertheless still mandatory to store the handle when receiving it in the cleanup hook, because it must be passed to napi_remove_async_cleanup_hook when the handle cleanup completes:

Current implementation                               Proposed implementation
|                                                    |
v                                                    v
+------------------------------------------------+   +----------------------------------------------+
| Create async thing (e.g. uv handle)            |   | Create async thing (e.g. uv handle)          |
| napi_add_async_cleanup_hook(hook_cb)           |   | napi_add_async_cleanup_hook(hook_cb)         |
| Optional: Store returned hook_handle           |   | Optional: Store returned hook_handle         |
+------------------------------------------------+   +----------------------------------------------+
|                                                    |
v                                                    v
+- hook_cb: -------------------------------------+   +- hook_cb: -----------------------------------+
| store done_cb (function ptr)                   | ! |                                              |
| store done_arg (void*)                         | ! |                                              |
| if a handle was stored                         | ! | if a handle was not stored                   |
|   call napi_remove_async_cleanup_hook(handle)  | ! |   store it for use in the uv_close cb        |
| Start closing the async thing (e.g uv_close)   |   | Start closing the async thing (e.g uv_close) |
+------------------------------------------------+   +----------------------------------------------+
|                                                    |
v                                                    v
+- uv_close cb: ---------------------------------+   +- uv_close cb: -------------------------------+
| call done_cb(done_arg)                         | ! | call napi_remove_async_cleanup_hook(handle)  |
+------------------------------------------------+   +----------------------------------------------+


+--+
|  | <-- an iteration of the event loop
+--+

This actually reduces the breakage slightly because the first step is now identical to the current implementation.

Please mind the edit above, sorry:

- "...the first step is not identical..."
+ "...the first step is now identical..."

CI: https://ci.nodejs.org/job/node-test-pull-request/32950/

CI: https://ci.nodejs.org/job/node-test-pull-request/32951/

LGTM

Landed in 0848f56.

* Avoid passing core `void*` and function pointers into add-on. * Document `napi_async_cleanup_hook_handle` type. * Render receipt of the handle mandatory from the point where the hook gets called. Removal of the handle remains mandatory. Fixes: #34715 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Anna Henningsen <github@addaleax.net> PR-URL: #34819 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

PR: nodejs/node#34819

nodejs/node#34819

* Avoid passing core `void*` and function pointers into add-on. * Document `napi_async_cleanup_hook_handle` type. * Render receipt of the handle mandatory from the point where the hook gets called. Removal of the handle remains mandatory. Fixes: #34715 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Anna Henningsen <github@addaleax.net> PR-URL: #34819 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 18, 2020

gabrielschulhof requested review from addaleax, himself65, legendecas and mhdawson August 18, 2020 05:29

gabrielschulhof commented Aug 18, 2020

View reviewed changes

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Aug 18, 2020

addaleax reviewed Aug 18, 2020

View reviewed changes

Gabriel Schulhof and others added 5 commits August 19, 2020 11:17

move handle differently

5986bab

Apply suggestions from code review

fc6e0b7

Co-authored-by: Anna Henningsen <github@addaleax.net>

add link to new callback type

2d7c4f2

add changes: sections

6037705

gabrielschulhof force-pushed the re-implement-async-cleanup-hooks branch from 5cf1c38 to 6037705 Compare August 19, 2020 18:23

himself65 approved these changes Aug 24, 2020

View reviewed changes

addaleax mentioned this pull request Aug 26, 2020

n-api,src: provide asynchronous cleanup hooks #34572

Closed

4 tasks

codebytere mentioned this pull request Aug 26, 2020

fix: provide asynchronous cleanup hooks in n-api electron/electron#25135

Merged

5 tasks

mhdawson reviewed Aug 26, 2020

View reviewed changes

mhdawson approved these changes Aug 26, 2020

View reviewed changes

storing the handle at creation is optional again

5b694e4

gabrielschulhof force-pushed the re-implement-async-cleanup-hooks branch from 0f4da9b to 5b694e4 Compare August 27, 2020 03:51

mhdawson approved these changes Aug 27, 2020

View reviewed changes

gabrielschulhof closed this Aug 27, 2020

gabrielschulhof deleted the re-implement-async-cleanup-hooks branch August 27, 2020 15:21

richardlau mentioned this pull request Sep 2, 2020

v14.10.0 proposal #35023

Merged

4 tasks

ianhattendorf added a commit to ianhattendorf/nodegit that referenced this pull request Sep 11, 2020

Test on node 14 only until backport of required node PR

9d5a42b

PR: nodejs/node#34819

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Remove redundant n-api patch

7a0f220

nodejs/node#34819

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Remove redundant n-api patch

f397de6

nodejs/node#34819

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Remove redundant n-api patch

b75271e

nodejs/node#34819

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Remove redundant n-api patch

ea664b3

nodejs/node#34819

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

gabrielschulhof mentioned this pull request Oct 7, 2020

Tracking for N-API PRs needing backport nodejs/abi-stable-node#406

Closed

coderabbitai Bot mentioned this pull request Aug 8, 2026

napi: keep async cleanup hook handles alive until the addon removes them oven-sh/bun#37204

Merged

-                node::RemoveEnvironmentCleanupHook(std::move(remove_handle->handle));
-                delete remove_handle;
+                if (remove_handle == nullptr)

n-api: re-implement async env cleanup hooks by gabrielschulhof · Pull Request #34819 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: re-implement async env cleanup hooks - #34819

n-api: re-implement async env cleanup hooks#34819gabrielschulhof wants to merge 6 commits intonodejs:masternodejs/node:masterfrom gabrielschulhof:re-implement-async-cleanup-hooksgabrielschulhof/node:re-implement-async-cleanup-hooksCopy head branch name to clipboard

Conversation

gabrielschulhof commented Aug 18, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 18, 2020

Uh oh!

gabrielschulhof Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

addaleax Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

mhdawson Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Aug 18, 2020

Uh oh!

addaleax commented Aug 18, 2020

Uh oh!

gabrielschulhof commented Aug 19, 2020

Uh oh!

gabrielschulhof commented Aug 19, 2020

Uh oh!

himself65 left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 26, 2020

Uh oh!

mhdawson Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Aug 26, 2020

Uh oh!

gabrielschulhof commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Aug 26, 2020

Uh oh!

nodejs-github-bot commented Aug 26, 2020

Uh oh!

nodejs-github-bot commented Aug 27, 2020

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

n-api: re-implement async env cleanup hooks#34819
gabrielschulhof wants to merge 6 commits into
nodejs:masterfrom
gabrielschulhof:re-implement-async-cleanup-hooks

gabrielschulhof commented Aug 26, 2020 •
edited
Loading