n-api: re-implement async env cleanup hooks

[gabrielschulhof]

• 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 [email protected]

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

[nodejs-github-bot]

Review requested:

• @nodejs/n-api

[gabrielschulhof]

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.

[addaleax]

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.

[addaleax]

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

[gabrielschulhof]

Added the link below.

[mhdawson]

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.

[gabrielschulhof]

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

[addaleax]

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

[gabrielschulhof]

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 +--+ 

[gabrielschulhof]

@addaleax I added the changes: sections you requested.

[himself65]

LGTM

[nodejs-github-bot]

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

[mhdawson]

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.

[mhdawson]

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

[mhdawson]

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.

[gabrielschulhof]

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

[gabrielschulhof]

Please mind the edit above, sorry:

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mhdawson]

LGTM

[gabrielschulhof]

Landed in 0848f56.