N-API: Some issues with the new async cleanup hooks

[gabrielschulhof]

At

node/src/node_api.h

Line 257 in bcfb176

void (*fun)(void* arg, void(* cb)(void*), void* cbarg),

cbarg is 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 to cb. There is no guarantee that add-on authors will faithfully pass it back as expected.

We should create a wrapper that retains cbarg in the implementation, making it possible to give a function `void (*cb)() to the add-on.

[gabrielschulhof]

@addaleax, please correct me if I'm wrong!

[gabrielschulhof]

It should be OK to make this change because the async cleanup hooks APIs are still NAPI_EXPERIMENTAL.

[addaleax]

We should create a wrapper that retains cbarg in 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 function napi_finish_async_cleanup_hook(env, cbarg) that would do the same thing as cb(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.

[addaleax]

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.

[gabrielschulhof]

@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_env is not Unref()-ed, potentially causing references to be leaked.

[addaleax]

@gabrielschulhof Quoting the docs (emphasis added by me):

If remove_handle is not NULL, 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.

[gabrielschulhof]

@addaleax Oh, sorry! Should've looked closer!

[mhdawson]

@gabrielschulhof mentioned that we could close the issue.

[gabrielschulhof]

@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_handle mandatory, and we can store the done_cb and the done_arg in the remove_handle, and call done_cb(done_arg) from inside napi_remove_async_cleanup_hook which must now be called from the uv_close cb above, thereby avoiding the need to expose the done_cb and the done_arg to 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::RemoveEnvironmentCleanupHook and calling done_cb()? If so, I would need to add that napi_finish_async_cleanup_hook you 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?

[addaleax]

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_cb and the done_arg to the add-on

I 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); or call_some_function(async_cleanup_handle); effectively doesn’t make a difference.

and call done_cb(done_arg) from inside napi_remove_async_cleanup_hook which must now be called from the uv_close cb above,

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::RemoveEnvironmentCleanupHook and calling done_cb()?

No, calling done_cb(done_arg) can currently also happen fully synchronously.

[addaleax]

I’ve also renamed the title as otherwise this sounds as if it were about async hooks :)

[gabrielschulhof]

thereby avoiding the need to expose the done_cb and the done_arg to the add-on

I 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); or call_some_function(async_cleanup_handle); effectively doesn’t make a difference.

AFAICT currently the add-on must store done_cb, done_arg, and possibly remove_handle whereas with my modification it would need only store remove_handle.