n-api: re-implement async env cleanup hooks

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

Author: Gabriel Schulhof

doc/api/n-api.md

@@ -619,6 +619,15 @@ typedef struct {
 } napi_type_tag;
 ```
 
+#### napi_async_cleanup_hook_handle
+<!-- YAML
+added: REPLACEME
+-->
+
+An opaque value returned by [`napi_add_async_cleanup_hook`][]. It must be passed
+to [`napi_remove_async_cleanup_hook`][] when the chain of asynchronous cleanup
+events completes.
+
 ### N-API callback types
 
 #### napi_callback_info
@@ -747,6 +756,28 @@ typedef void (*napi_threadsafe_function_call_js)(napi_env env,
 Unless for reasons discussed in [Object Lifetime Management][], creating a
 handle and/or callback scope inside the function body is not necessary.
 
+#### napi_async_cleanup_hook
+<!-- YAML
+added: REPLACEME
+-->
+
+Function pointer used with [`napi_add_async_cleanup_hook`][]. It will be called
+when the environment is being torn down.
+
+Callback functions must satisfy the following signature:
+
+```c
+typedef void (*napi_async_cleanup_hook)(napi_async_cleanup_hook_handle handle,
+                                        void* data);
+```
+
+* `[in] handle`: The handle obtained from [`napi_add_async_cleanup_hook`][].
+* `[in] data`: The data that was passed to [`napi_add_async_cleanup_hook`][].
+
+The body of the function should initiate the asynchronous cleanup actions at the
+end of which `handle` must be passed in a call to
+[`napi_remove_async_cleanup_hook`][].
+
 ## Error handling
 
 N-API uses both return values and JavaScript exceptions for error handling.
@@ -1583,23 +1614,30 @@ added: v14.8.0
 ```c
 NAPI_EXTERN napi_status napi_add_async_cleanup_hook(
     napi_env env,
-    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    napi_async_cleanup_hook hook,
     void* arg,
     napi_async_cleanup_hook_handle* remove_handle);
 ```
 
-Registers `fun` as a function to be run with the `arg` parameter once the
-current Node.js environment exits. Unlike [`napi_add_env_cleanup_hook`][],
-the hook is allowed to be asynchronous in this case, and must invoke the passed
-`cb()` function with `cbarg` once all asynchronous activity is finished.
+* `[in] env`: The environment that the API is invoked under.
+* `[in] hook`: The function pointer to call at environment teardown.
+* `[in] arg`: The pointer to pass to `hook` when it gets called.
+* `[out] remove_handle`: The handle that refers to the asynchronous cleanup
+hook. This handle must be passed to [`napi_remove_async_cleanup_hook`][] even if
+`hook` gets called.
+
+Registers `hook` as a function to be run with the `remove_handle` and `arg`
+parameters once the current Node.js environment exits. Unlike
+[`napi_add_env_cleanup_hook`][], the hook is allowed to be asynchronous, and
+must pass `remove_handle` in a call to [`napi_remove_env_cleanup_hook`][] once
+all asynchronous activity is finished.
 
 Otherwise, behavior generally matches that of [`napi_add_env_cleanup_hook`][].
 
-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.
-Typically, that happens when the resource for which this hook was added
-is being torn down anyway.
+An opaque value will be stored in `remove_handle` that must later be passed to
+[`napi_remove_async_cleanup_hook`][], regardless of whether the hook has already
+been invoked. Typically, that happens when the resource for which this hook was
+added is being torn down anyway.
 
 #### napi_remove_async_cleanup_hook
 <!-- YAML
@@ -1610,13 +1648,15 @@ added: v14.8.0
 
 ```c
 NAPI_EXTERN napi_status napi_remove_async_cleanup_hook(
-    napi_env env,
     napi_async_cleanup_hook_handle remove_handle);
 ```
 
+* `[in] remove_handle`: The handle to an asynchronous cleanup hook that was
+created with [`napi_add_async_cleanup_hook`][].
+
 Unregisters the cleanup hook corresponding to `remove_handle`. This will prevent
 the hook from being executed, unless it has already started executing.
-This must be called on any `napi_async_cleanup_hook_handle` value retrieved
+This must be called on any `napi_async_cleanup_hook_handle` value obtained
 from [`napi_add_async_cleanup_hook`][].
 
 ## Module registration

src/node_api.cc

@@ -519,41 +519,66 @@ napi_status napi_remove_env_cleanup_hook(napi_env env,
 }
 
 struct napi_async_cleanup_hook_handle__ {
-  node::AsyncCleanupHookHandle handle;
+  napi_async_cleanup_hook_handle__(napi_env env,
+                                   napi_async_cleanup_hook user_hook,
+                                   void* user_data):
+      handle_(std::move(node::AddEnvironmentCleanupHook(env->isolate,
+                                                        Hook,
+                                                        this))),
+      env_(env),
+      user_hook_(user_hook),
+      user_data_(user_data) {
+    env->Ref();
+  }
+
+  ~napi_async_cleanup_hook_handle__() {
+    node::RemoveEnvironmentCleanupHook(std::move(handle_));
+    if (done_cb_)
+      done_cb_(done_data_);
+
+    // Release the `env` handle asynchronously since it would be surprising if
+    // a call to a N-API function would destroy `env` synchronously.
+    static_cast<node_napi_env>(env_)->node_env()
+        ->SetImmediate([env = env_](node::Environment*) { env->Unref(); });
+  }
+
+  static void Hook(void* data, void (*done_cb)(void*), void* done_data) {
+    auto handle = static_cast<napi_async_cleanup_hook_handle__*>(data);
+    handle->done_cb_ = done_cb;
+    handle->done_data_ = done_data;
+    handle->user_hook_(handle, handle->user_data_);
+  }
+
+  node::AsyncCleanupHookHandle handle_;
+  napi_env env_ = nullptr;
+  napi_async_cleanup_hook user_hook_ = nullptr;
+  void* user_data_ = nullptr;
+  void (*done_cb_)(void*) = nullptr;
+  void* done_data_ = nullptr;
 };
 
 napi_status napi_add_async_cleanup_hook(
     napi_env env,
-    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    napi_async_cleanup_hook hook,
     void* arg,
     napi_async_cleanup_hook_handle* remove_handle) {
   CHECK_ENV(env);
-  CHECK_ARG(env, fun);
-
-  auto handle = node::AddEnvironmentCleanupHook(env->isolate, fun, arg);
-  if (remove_handle != nullptr) {
-    *remove_handle = new napi_async_cleanup_hook_handle__ { std::move(handle) };
-    env->Ref();
-  }
+  CHECK_ARG(env, hook);
+  CHECK_ARG(env, remove_handle);
 
+  *remove_handle = new napi_async_cleanup_hook_handle__(env, hook, arg);
   return napi_clear_last_error(env);
 }
 
 napi_status napi_remove_async_cleanup_hook(
-    napi_env env,
     napi_async_cleanup_hook_handle remove_handle) {
-  CHECK_ENV(env);
-  CHECK_ARG(env, remove_handle);
 
-  node::RemoveEnvironmentCleanupHook(std::move(remove_handle->handle));
-  delete remove_handle;
+  if (remove_handle == nullptr)
+    return napi_invalid_arg;
 
-  // Release the `env` handle asynchronously since it would be surprising if
-  // a call to a N-API function would destroy `env` synchronously.
-  static_cast<node_napi_env>(env)->node_env()
-      ->SetImmediate([env](node::Environment*) { env->Unref(); });
+  delete remove_handle;
 
-  return napi_clear_last_error(env);
+  return napi_ok;
 }
 
 napi_status napi_fatal_exception(napi_env env, napi_value err) {

src/node_api.h

@@ -254,12 +254,11 @@ napi_ref_threadsafe_function(napi_env env, napi_threadsafe_function func);
 
 NAPI_EXTERN napi_status napi_add_async_cleanup_hook(
     napi_env env,
-    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    napi_async_cleanup_hook hook,
     void* arg,
     napi_async_cleanup_hook_handle* remove_handle);
 
 NAPI_EXTERN napi_status napi_remove_async_cleanup_hook(
-    napi_env env,
     napi_async_cleanup_hook_handle remove_handle);
 
 #endif  // NAPI_EXPERIMENTAL

src/node_api_types.h

@@ -43,6 +43,8 @@ typedef struct {
 
 #ifdef NAPI_EXPERIMENTAL
 typedef struct napi_async_cleanup_hook_handle__* napi_async_cleanup_hook_handle;
+typedef void (*napi_async_cleanup_hook)(napi_async_cleanup_hook_handle handle,
+                                        void* data);
 #endif  // NAPI_EXPERIMENTAL
 
 #endif  // SRC_NODE_API_TYPES_H_

test/node-api/test_async_cleanup_hook/binding.c

@@ -5,44 +5,34 @@
 #include <stdlib.h>
 #include "../../js-native-api/common.h"
 
-void MustNotCall(void* arg, void(*cb)(void*), void* cbarg) {
+static void MustNotCall(napi_async_cleanup_hook_handle hook, void* arg) {
   assert(0);
 }
 
 struct AsyncData {
   uv_async_t async;
   napi_env env;
   napi_async_cleanup_hook_handle handle;
-  void (*done_cb)(void*);
-  void* done_arg;
 };
 
-struct AsyncData* CreateAsyncData() {
+static struct AsyncData* CreateAsyncData() {
   struct AsyncData* data = (struct AsyncData*) malloc(sizeof(struct AsyncData));
   data->handle = NULL;
   return data;
 }
 
-void AfterCleanupHookTwo(uv_handle_t* handle) {
+static void AfterCleanupHookTwo(uv_handle_t* handle) {
   struct AsyncData* data = (struct AsyncData*) handle->data;
-  data->done_cb(data->done_arg);
+  napi_status status = napi_remove_async_cleanup_hook(data->handle);
+  assert(status == napi_ok);
   free(data);
 }
 
-void AfterCleanupHookOne(uv_async_t* async) {
-  struct AsyncData* data = (struct AsyncData*) async->data;
-  if (data->handle != NULL) {
-    // Verify that removing the hook is okay between starting and finishing
-    // of its execution.
-    napi_status status =
-        napi_remove_async_cleanup_hook(data->env, data->handle);
-    assert(status == napi_ok);
-  }
-
+static void AfterCleanupHookOne(uv_async_t* async) {
   uv_close((uv_handle_t*) async, AfterCleanupHookTwo);
 }
 
-void AsyncCleanupHook(void* arg, void(*cb)(void*), void* cbarg) {
+static void AsyncCleanupHook(napi_async_cleanup_hook_handle handle, void* arg) {
   struct AsyncData* data = (struct AsyncData*) arg;
   uv_loop_t* loop;
   napi_status status = napi_get_uv_event_loop(data->env, &loop);
@@ -51,29 +41,21 @@ void AsyncCleanupHook(void* arg, void(*cb)(void*), void* cbarg) {
   assert(err == 0);
 
   data->async.data = data;
-  data->done_cb = cb;
-  data->done_arg = cbarg;
   uv_async_send(&data->async);
 }
 
-napi_value Init(napi_env env, napi_value exports) {
+static napi_value Init(napi_env env, napi_value exports) {
   {
     struct AsyncData* data = CreateAsyncData();
     data->env = env;
     napi_add_async_cleanup_hook(env, AsyncCleanupHook, data, &data->handle);
   }
 
-  {
-    struct AsyncData* data = CreateAsyncData();
-    data->env = env;
-    napi_add_async_cleanup_hook(env, AsyncCleanupHook, data, NULL);
-  }
-
   {
     napi_async_cleanup_hook_handle must_not_call_handle;
     napi_add_async_cleanup_hook(
         env, MustNotCall, NULL, &must_not_call_handle);
-    napi_remove_async_cleanup_hook(env, must_not_call_handle);
+    napi_remove_async_cleanup_hook(must_not_call_handle);
   }
 
   return NULL;