Repository navigation
N-API: Some issues with the new async cleanup hooks #34715
Description
Activity
@addaleax, please correct me if I'm wrong!
It should be OK to make this change because the async cleanup hooks APIs are still
NAPI_EXPERIMENTAL.We should create a wrapper that retains
cbargin the implementation, making it possible to give a function `void (*cb)() to the add-on.That would require bringing closures into C99 :)
The argument that we could potentially elide is
cb, and instead create a new functionnapi_finish_async_cleanup_hook(env, cbarg)that would do the same thing ascb(cbarg);. I don’t have a strong opinion about that, it might force us to add another heap allocation at some point in the future if the internals ever change.Also, this isn’t relevant for wrapper APIs in high-level languages like C++ or Rust where the callback would be represented as a closure anyway.
There is no guarantee that add-on authors will faithfully pass it back as expected.
Also: That’s correct, but the addon author should notice pretty quickly that their tests are broken if they don’t.
- changed the title
[-]We should not expose the void* coming in via the async cleanup hook to the add-on[/-][+]Some issues with the new async hooks[/+]on Aug 12, 2020 @addaleax widening the scope a bit.
If the hook actually fires, AFAICT- the heap-allocated
napi_async_cleanup_hook_handle__structure is not freed, and - the
napi_envis notUnref()-ed, potentially causing references to be leaked.
- the heap-allocated
@gabrielschulhof Quoting the docs (emphasis added by me):
If
remove_handleis notNULL, an opaque value will be stored in it
that must later be passed to [napi_remove_async_cleanup_hook][],
regardless of whether the hook has already been invoked.@addaleax Oh, sorry! Should've looked closer!
@gabrielschulhof mentioned that we could close the issue.
@mhdawson sorry, another question came to mind for @addaleax so I'm reopening:
@addaleax would the async cleanup hooks still function correctly if we modified the sequence of handling them as follows? "Before" is the way it is currently done in the test (AFAICT), whereas "After" is what I'm thinking about:
Before: After: | | v v + cleanup_hook: -------------------+ + cleanup_hook: -------------------+ | store done_cb | | store done_cb | | call async_init | | call async_init | | call async_send | | call async_send | +----------------------------------+ +----------------------------------+ | | v v + async_send cb: ------------------+ + async_send cb: ------------------+ | call node::RmEnvClHook via N-API | | call uv_close | | call uv_close | +----------------------------------+ +----------------------------------+ | | v v + uv_close cb: --------------------+ + uv_close cb: --------------------+ | call node::RmEnvClHook via N-API | | call done_cb | | call done_cb | +----------------------------------+ +----------------------------------+If so, we can change the API to make receiving the
remove_handlemandatory, and we can store thedone_cband thedone_argin theremove_handle, and calldone_cb(done_arg)from insidenapi_remove_async_cleanup_hookwhich must now be called from theuv_close cbabove, thereby avoiding the need to expose thedone_cband thedone_argto the add-on. I've got a branch going with such a change, but I wanted to double-check with you before going too far down the rabbit hole.I guess the basic question is this: Must there be an event loop iteration between calling
node::RemoveEnvironmentCleanupHookand callingdone_cb()? If so, I would need to add thatnapi_finish_async_cleanup_hookyou mentioned earlier to perform the in-between step.There is also a simpler sequence for closing an a priori handle the async cleanup hooks should address. This is how I imagine my modification would change the way that use case is addressed:
Before: After: | | v v + cleanup_hook: -------------------+ + cleanup_hook: -------------------+ | store done_cb | | store done_cb | | call node::RmEnvClHook via N-API | | call node::RmEnvClHook via N-API | | call uv_close (a priori handle) | | call uv_close (a priori handle) | +----------------------------------+ +----------------------------------+ | | v v + uv_close cb: --------------------+ + uv_close cb: --------------------+ | call done_cb | | call node::RmEnvClHook via N-API | +----------------------------------+ | call done_cb | +----------------------------------+WDYT?
would the async cleanup hooks still function correctly if we modified the sequence of handling them as follows?
It should work the same.
thereby avoiding the need to expose the
done_cband thedone_argto the add-onI still don’t see how that’s noticeably better – the addon has to do one specific thing, and whether that’s
done_cb(done_arg);orcall_some_function(async_cleanup_handle);effectively doesn’t make a difference.and call
done_cb(done_arg)from insidenapi_remove_async_cleanup_hookwhich must now be called from theuv_close cbabove,Keep in mind that that changes the internals quite a bit, as
napi_remove_async_cleanup_hook()still needs to be callable before the hook is ever entered.Must there be an event loop iteration between calling
node::RemoveEnvironmentCleanupHookand callingdone_cb()?No, calling
done_cb(done_arg)can currently also happen fully synchronously.- changed the title
[-]Some issues with the new async hooks[/-][+]N-API: Some issues with the new async cleanup hooks[/+]on Aug 17, 2020 I’ve also renamed the title as otherwise this sounds as if it were about async hooks :)
thereby avoiding the need to expose the
done_cband thedone_argto the add-onI still don’t see how that’s noticeably better – the addon has to do one specific thing, and whether that’s
done_cb(done_arg);orcall_some_function(async_cleanup_handle);effectively doesn’t make a difference.AFAICT currently the add-on must store
done_cb,done_arg, and possiblyremove_handlewhereas with my modification it would need only storeremove_handle.- added a commit that references this issue
on Sep 1, 2020 - added a commit that references this issue
on Sep 26, 2020 - added a commit that references this issue
on May 22, 2026
At
node/src/node_api.h
Line 257 in bcfb176
cbargis a pointer passing from the core into the add-on. The core expects that it will be passed back into the core as the first parameter tocb. There is no guarantee that add-on authors will faithfully pass it back as expected.We should create a wrapper that retains
cbargin the implementation, making it possible to give a function `void (*cb)() to the add-on.