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 from the point where the
  hook gets called. Removal of the handle remains mandatory.

Fixes: https://github.com/nodejs/node/issues/34715
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: https://github.com/nodejs/node/pull/34819
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>

5 files changed, 116 insertions, 48 deletions

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
index dd4f1329b4e..91341a2f6c5 100644
--- a/doc/api/n-api.md
+++ b/doc/api/n-api.md
@@ -623,6 +623,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
@@ -751,6 +760,30 @@ 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 that must be passed to
+[`napi_remove_async_cleanup_hook`][] after completion of the asynchronous
+cleanup.
+* `[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.
@@ -1580,6 +1613,10 @@ with `napi_add_env_cleanup_hook`, otherwise the process will abort.
 #### napi_add_async_cleanup_hook
 <!-- YAML
 added: v14.8.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/34819
+    description: Changed signature of the `hook` callback.
 -->
 
 > Stability: 1 - Experimental
@@ -1587,15 +1624,22 @@ 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`: Optional handle that refers to the asynchronous cleanup
+hook.
+
+Registers `hook`, which is a function of type [`napi_async_cleanup_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.
 
 Otherwise, behavior generally matches that of [`napi_add_env_cleanup_hook`][].
 
@@ -1608,19 +1652,25 @@ is being torn down anyway.
 #### napi_remove_async_cleanup_hook
 <!-- YAML
 added: v14.8.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/34819
+    description: Removed `env` parameter.
 -->
 
 > Stability: 1 - Experimental
 
 ```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
@@ -5763,6 +5813,7 @@ This API may only be called from the main thread.
 [`napi_add_async_cleanup_hook`]: #n_api_napi_add_async_cleanup_hook
 [`napi_add_env_cleanup_hook`]: #n_api_napi_add_env_cleanup_hook
 [`napi_add_finalizer`]: #n_api_napi_add_finalizer
+[`napi_async_cleanup_hook`]: #n_api_napi_async_cleanup_hook
 [`napi_async_complete_callback`]: #n_api_napi_async_complete_callback
 [`napi_async_init`]: #n_api_napi_async_init
 [`napi_callback`]: #n_api_napi_callback
diff --git a/src/node_api.cc b/src/node_api.cc
index 4fbab771d58..93488146d56 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -519,41 +519,68 @@ 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):
+      env_(env),
+      user_hook_(user_hook),
+      user_data_(user_data) {
+    handle_ = node::AddEnvironmentCleanupHook(env->isolate, Hook, this);
+    env->Ref();
+  }
+
+  ~napi_async_cleanup_hook_handle__() {
+    node::RemoveEnvironmentCleanupHook(std::move(handle_));
+    if (done_cb_ != nullptr)
+      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);
+  CHECK_ARG(env, hook);
 
-  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();
-  }
+  napi_async_cleanup_hook_handle__* handle =
+    new napi_async_cleanup_hook_handle__(env, hook, arg);
+
+  if (remove_handle != nullptr)
+    *remove_handle = handle;
 
   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) {
diff --git a/src/node_api.h b/src/node_api.h
index 4f3eb8f2caa..577a1dcd949 100644
--- a/src/node_api.h
+++ b/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
diff --git a/src/node_api_types.h b/src/node_api_types.h
index b8711d3eddc..0e400e9676d 100644
--- a/src/node_api_types.h
+++ b/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_
diff --git a/test/node-api/test_async_cleanup_hook/binding.c b/test/node-api/test_async_cleanup_hook/binding.c
index f0c9cd97a26..7bbde56bb0e 100644
--- a/test/node-api/test_async_cleanup_hook/binding.c
+++ b/test/node-api/test_async_cleanup_hook/binding.c
@@ -5,7 +5,7 @@
 #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);
 }
 
@@ -13,36 +13,26 @@ 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,12 +41,11 @@ void AsyncCleanupHook(void* arg, void(*cb)(void*), void* cbarg) {
   assert(err == 0);
 
   data->async.data = data;
-  data->done_cb = cb;
-  data->done_arg = cbarg;
+  data->handle = handle;
   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;
@@ -73,7 +62,7 @@ napi_value Init(napi_env env, napi_value exports) {
     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;