n-api,src: provide asynchronous cleanup hooks

Sometimes addons need to perform cleanup actions, for example closing libuv handles or waiting for requests to finish, that cannot be performed synchronously. Add C++ API and N-API functions that allow providing such asynchronous cleanup hooks. Fixes: #34567 PR-URL: #34572 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
nodejs · codebytere · Aug 11, 2020 · Jul 16, 2020 · Jul 23, 2020 · Jul 27, 2020
commit 8630f3477697835719df93dbc49d03f60cdf2b31
diff --git a/doc/api/addons.md b/doc/api/addons.md
@@ -232,6 +232,12 @@ NODE_MODULE_INIT(/* exports, module, context */) {
 ```
 
 #### Worker support
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/34572
+    description: Cleanup hooks may now be asynchronous.
+-->
 
 In order to be loaded from multiple Node.js environments,
 such as a main thread and a Worker thread, an add-on needs to either:
@@ -254,6 +260,11 @@ down. If necessary, such hooks can be removed using
 `RemoveEnvironmentCleanupHook()` before they are run, which has the same
 signature. Callbacks are run in last-in first-out order.
 
+If necessary, there is an additional pair of `AddEnvironmentCleanupHook()`
+and `RemoveEnvironmentCleanupHook()` overloads, where the cleanup hook takes a
+callback function. This can be used for shutting down asynchronous resources,
+for example any libuv handles registered by the addon.
+
 The following `addon.cc` uses `AddEnvironmentCleanupHook`:
 
 ```cpp

diff --git a/doc/api/n-api.md b/doc/api/n-api.md
@@ -1550,10 +1550,12 @@ and will lead the process to abort.
 The hooks will be called in reverse order, i.e. the most recently added one
 will be called first.
 
-Removing this hook can be done by using `napi_remove_env_cleanup_hook`.
+Removing this hook can be done by using [`napi_remove_env_cleanup_hook`][].
 Typically, that happens when the resource for which this hook was added
 is being torn down anyway.
 
+For asynchronous cleanup, [`napi_add_async_cleanup_hook`][] is available.
+
 #### napi_remove_env_cleanup_hook
 <!-- YAML
 added: v10.2.0
@@ -1573,6 +1575,52 @@ need to be exact matches.
 The function must have originally been registered
 with `napi_add_env_cleanup_hook`, otherwise the process will abort.
 
+#### napi_add_async_cleanup_hook
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+```c
+NAPI_EXTERN napi_status napi_add_async_cleanup_hook(
+    napi_env env,
+    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    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.
+
+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.
+
+#### napi_remove_async_cleanup_hook
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+```c
+NAPI_EXTERN napi_status napi_remove_async_cleanup_hook(
+    napi_env env,
+    napi_async_cleanup_hook_handle remove_handle);
+```
+
+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
+from [`napi_add_async_cleanup_hook`][].
+
 ## Module registration
 N-API modules are registered in a manner similar to other modules
 except that instead of using the `NODE_MODULE` macro the following
@@ -5704,6 +5752,7 @@ This API may only be called from the main thread.
 [`Worker`]: worker_threads.html#worker_threads_class_worker
 [`global`]: globals.html#globals_global
 [`init` hooks]: async_hooks.html#async_hooks_init_asyncid_type_triggerasyncid_resource
+[`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_complete_callback`]: #n_api_napi_async_complete_callback
@@ -5744,6 +5793,8 @@ This API may only be called from the main thread.
 [`napi_queue_async_work`]: #n_api_napi_queue_async_work
 [`napi_reference_ref`]: #n_api_napi_reference_ref
 [`napi_reference_unref`]: #n_api_napi_reference_unref
+[`napi_remove_async_cleanup_hook`]: #n_api_napi_remove_async_cleanup_hook
+[`napi_remove_env_cleanup_hook`]: #n_api_napi_remove_env_cleanup_hook
 [`napi_set_instance_data`]: #n_api_napi_set_instance_data
 [`napi_set_property`]: #n_api_napi_set_property
 [`napi_threadsafe_function_call_js`]: #n_api_napi_threadsafe_function_call_js

diff --git a/src/api/hooks.cc b/src/api/hooks.cc
@@ -73,22 +73,86 @@ int EmitExit(Environment* env) {
       .ToChecked();
 }
 
+typedef void (*CleanupHook)(void* arg);
+typedef void (*AsyncCleanupHook)(void* arg, void(*)(void*), void*);
+
+struct AsyncCleanupHookInfo final {
+  Environment* env;
+  AsyncCleanupHook fun;
+  void* arg;
+  bool started = false;
+  // Use a self-reference to make sure the storage is kept alive while the
+  // cleanup hook is registered but not yet finished.
+  std::shared_ptr<AsyncCleanupHookInfo> self;
+};
+
+// Opaque type that is basically an alias for `shared_ptr<AsyncCleanupHookInfo>`
+// (but not publicly so for easier ABI/API changes). In particular,
+// std::shared_ptr does not generally maintain a consistent ABI even on a
+// specific platform.
+struct ACHHandle final {
+  std::shared_ptr<AsyncCleanupHookInfo> info;
+};
+// This is implemented as an operator on a struct because otherwise you can't
+// default-initialize AsyncCleanupHookHandle, because in C++ for a
+// std::unique_ptr to be default-initializable the deleter type also needs
+// to be default-initializable; in particular, function types don't satisfy
+// this.
+void DeleteACHHandle::operator ()(ACHHandle* handle) const { delete handle; }
+
 void AddEnvironmentCleanupHook(Isolate* isolate,
-                               void (*fun)(void* arg),
+                               CleanupHook fun,
                                void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
   CHECK_NOT_NULL(env);
   env->AddCleanupHook(fun, arg);
 }
 
 void RemoveEnvironmentCleanupHook(Isolate* isolate,
-                                  void (*fun)(void* arg),
+                                  CleanupHook fun,
                                   void* arg) {
   Environment* env = Environment::GetCurrent(isolate);
   CHECK_NOT_NULL(env);
   env->RemoveCleanupHook(fun, arg);
 }
 
+static void FinishAsyncCleanupHook(void* arg) {
+  AsyncCleanupHookInfo* info = static_cast<AsyncCleanupHookInfo*>(arg);
+  std::shared_ptr<AsyncCleanupHookInfo> keep_alive = info->self;
+
+  info->env->DecreaseWaitingRequestCounter();
+  info->self.reset();
+}
+
+static void RunAsyncCleanupHook(void* arg) {
+  AsyncCleanupHookInfo* info = static_cast<AsyncCleanupHookInfo*>(arg);
+  info->env->IncreaseWaitingRequestCounter();
+  info->started = true;
+  info->fun(info->arg, FinishAsyncCleanupHook, info);
+}
+
+AsyncCleanupHookHandle AddEnvironmentCleanupHook(
+    Isolate* isolate,
+    AsyncCleanupHook fun,
+    void* arg) {
+  Environment* env = Environment::GetCurrent(isolate);
+  CHECK_NOT_NULL(env);
+  auto info = std::make_shared<AsyncCleanupHookInfo>();
+  info->env = env;
+  info->fun = fun;
+  info->arg = arg;
+  info->self = info;
+  env->AddCleanupHook(RunAsyncCleanupHook, info.get());
+  return AsyncCleanupHookHandle(new ACHHandle { info });
+}
+
+void RemoveEnvironmentCleanupHook(
+    AsyncCleanupHookHandle handle) {
+  if (handle->info->started) return;
+  handle->info->self.reset();
+  handle->info->env->RemoveCleanupHook(RunAsyncCleanupHook, handle->info.get());
+}
+
 async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
   Environment* env = Environment::GetCurrent(isolate);
   if (env == nullptr) return -1;

diff --git a/src/node.h b/src/node.h
@@ -872,6 +872,20 @@ NODE_EXTERN void RemoveEnvironmentCleanupHook(v8::Isolate* isolate,
                                               void (*fun)(void* arg),
                                               void* arg);
 
+/* These are async equivalents of the above. After the cleanup hook is invoked,
+ * `cb(cbarg)` *must* be called, and attempting to remove the cleanup hook will
+ * have no effect. */
+struct ACHHandle;
+struct NODE_EXTERN DeleteACHHandle { void operator()(ACHHandle*) const; };
+typedef std::unique_ptr<ACHHandle, DeleteACHHandle> AsyncCleanupHookHandle;
+
+NODE_EXTERN AsyncCleanupHookHandle AddEnvironmentCleanupHook(
+    v8::Isolate* isolate,
+    void (*fun)(void* arg, void (*cb)(void*), void* cbarg),
+    void* arg);
+
+NODE_EXTERN void RemoveEnvironmentCleanupHook(AsyncCleanupHookHandle holder);
+
 /* Returns the id of the current execution context. If the return value is
  * zero then no execution has been set. This will happen if the user handles
  * I/O from native code. */

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -518,6 +518,38 @@ napi_status napi_remove_env_cleanup_hook(napi_env env,
   return napi_ok;
 }
 
+struct napi_async_cleanup_hook_handle__ {
+  node::AsyncCleanupHookHandle handle;
+};
+
+napi_status napi_add_async_cleanup_hook(
+    napi_env env,
+    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    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) };
+  }
+
+  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;
+
+  return napi_clear_last_error(env);
+}
+
 napi_status napi_fatal_exception(napi_env env, napi_value err) {
   NAPI_PREAMBLE(env);
   CHECK_ARG(env, err);

diff --git a/src/node_api.h b/src/node_api.h
@@ -250,6 +250,20 @@ napi_ref_threadsafe_function(napi_env env, napi_threadsafe_function func);
 
 #endif  // NAPI_VERSION >= 4
 
+#ifdef NAPI_EXPERIMENTAL
+
+NAPI_EXTERN napi_status napi_add_async_cleanup_hook(
+    napi_env env,
+    void (*fun)(void* arg, void(* cb)(void*), void* cbarg),
+    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
+
 EXTERN_C_END
 
 #endif  // SRC_NODE_API_H_
diff --git a/src/node_api_types.h b/src/node_api_types.h
@@ -41,4 +41,8 @@ typedef struct {
   const char* release;
 } napi_node_version;
 
+#ifdef NAPI_EXPERIMENTAL
+typedef struct napi_async_cleanup_hook_handle__* napi_async_cleanup_hook_handle;
+#endif  // NAPI_EXPERIMENTAL
+
 #endif  // SRC_NODE_API_TYPES_H_
diff --git a/test/addons/async-cleanup-hook/binding.cc b/test/addons/async-cleanup-hook/binding.cc
@@ -0,0 +1,59 @@
+#include <assert.h>
+#include <node.h>
+#include <uv.h>
+
+void MustNotCall(void* arg, void(*cb)(void*), void* cbarg) {
+  assert(0);
+}
+
+struct AsyncData {
+  uv_async_t async;
+  v8::Isolate* isolate;
+  node::AsyncCleanupHookHandle handle;
+  void (*done_cb)(void*);
+  void* done_arg;
+};
+
+void AsyncCleanupHook(void* arg, void(*cb)(void*), void* cbarg) {
+  AsyncData* data = static_cast<AsyncData*>(arg);
+  uv_loop_t* loop = node::GetCurrentEventLoop(data->isolate);
+  assert(loop != nullptr);
+  int err = uv_async_init(loop, &data->async, [](uv_async_t* async) {
+    AsyncData* data = static_cast<AsyncData*>(async->data);
+    // Attempting to remove the cleanup hook here should be a no-op since it
+    // has already been started.
+    node::RemoveEnvironmentCleanupHook(std::move(data->handle));
+
+    uv_close(reinterpret_cast<uv_handle_t*>(async), [](uv_handle_t* handle) {
+      AsyncData* data = static_cast<AsyncData*>(handle->data);
+      data->done_cb(data->done_arg);
+      delete data;
+    });
+  });
+  assert(err == 0);
+
+  data->async.data = data;
+  data->done_cb = cb;
+  data->done_arg = cbarg;
+  uv_async_send(&data->async);
+}
+
+void Initialize(v8::Local<v8::Object> exports,
+                v8::Local<v8::Value> module,
+                v8::Local<v8::Context> context) {
+  AsyncData* data = new AsyncData();
+  data->isolate = context->GetIsolate();
+  auto handle = node::AddEnvironmentCleanupHook(
+      context->GetIsolate(),
+      AsyncCleanupHook,
+      data);
+  data->handle = std::move(handle);
+
+  auto must_not_call_handle = node::AddEnvironmentCleanupHook(
+      context->GetIsolate(),
+      MustNotCall,
+      nullptr);
+  node::RemoveEnvironmentCleanupHook(std::move(must_not_call_handle));
+}
+
+NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
diff --git a/test/addons/async-cleanup-hook/binding.gyp b/test/addons/async-cleanup-hook/binding.gyp
@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}
diff --git a/test/addons/async-cleanup-hook/test.js b/test/addons/async-cleanup-hook/test.js
@@ -0,0 +1,8 @@
+'use strict';
+const common = require('../../common');
+const path = require('path');
+const { Worker } = require('worker_threads');
+const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
+
+const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true });
+w.on('exit', common.mustCall(() => require(binding)));