From 2cf231678b5fa4b7acd849615b60b7004cfee455 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 31 Jul 2020 02:10:14 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/34567 PR-URL: https://github.com/nodejs/node/pull/34572 Reviewed-By: James M Snell Reviewed-By: Gabriel Schulhof --- doc/api/addons.md | 11 +++ doc/api/n-api.md | 53 +++++++++++- src/api/hooks.cc | 68 ++++++++++++++- src/node.h | 14 ++++ src/node_api.cc | 32 ++++++++ src/node_api.h | 14 ++++ src/node_api_types.h | 4 + test/addons/async-cleanup-hook/binding.cc | 59 +++++++++++++ test/addons/async-cleanup-hook/binding.gyp | 9 ++ test/addons/async-cleanup-hook/test.js | 8 ++ .../test_async_cleanup_hook/binding.c | 82 +++++++++++++++++++ .../test_async_cleanup_hook/binding.gyp | 9 ++ test/node-api/test_async_cleanup_hook/test.js | 8 ++ 13 files changed, 368 insertions(+), 3 deletions(-) create mode 100644 test/addons/async-cleanup-hook/binding.cc create mode 100644 test/addons/async-cleanup-hook/binding.gyp create mode 100644 test/addons/async-cleanup-hook/test.js create mode 100644 test/node-api/test_async_cleanup_hook/binding.c create mode 100644 test/node-api/test_async_cleanup_hook/binding.gyp create mode 100644 test/node-api/test_async_cleanup_hook/test.js diff --git a/doc/api/addons.md b/doc/api/addons.md index cc85a0fef6ad43..422cf7222d6c7c 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -234,6 +234,12 @@ NODE_MODULE_INIT(/* exports, module, context */) { ``` #### Worker support + 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: @@ -256,6 +262,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 index afb15bfea7214b..f529a07d4a76d1 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1543,10 +1543,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 + +> 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 + + +> 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 @@ -5659,6 +5707,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 @@ -5699,6 +5748,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 index 037bdda6f41c82..3b16c0350d8a84 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -73,8 +73,35 @@ 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 self; +}; + +// Opaque type that is basically an alias for `shared_ptr` +// (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 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); @@ -82,13 +109,50 @@ void AddEnvironmentCleanupHook(Isolate* isolate, } 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(arg); + std::shared_ptr keep_alive = info->self; + + info->env->DecreaseWaitingRequestCounter(); + info->self.reset(); +} + +static void RunAsyncCleanupHook(void* arg) { + AsyncCleanupHookInfo* info = static_cast(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(); + 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 index aba821a5e03f37..e9cf73ede5373a 100644 --- a/src/node.h +++ b/src/node.h @@ -735,6 +735,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 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 index 727e18f6767fc7..f57e879314733f 100644 --- 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 index 2f1b45572d8130..4f3eb8f2caae63 100644 --- 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 index 1c9a2b8aa21889..b8711d3eddc408 100644 --- 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 new file mode 100644 index 00000000000000..d18da7a094f71a --- /dev/null +++ b/test/addons/async-cleanup-hook/binding.cc @@ -0,0 +1,59 @@ +#include +#include +#include + +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(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(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(async), [](uv_handle_t* handle) { + AsyncData* data = static_cast(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 exports, + v8::Local module, + v8::Local 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 new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..55cc2517a59bc8 --- /dev/null +++ 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))); diff --git a/test/node-api/test_async_cleanup_hook/binding.c b/test/node-api/test_async_cleanup_hook/binding.c new file mode 100644 index 00000000000000..f0c9cd97a26c48 --- /dev/null +++ b/test/node-api/test_async_cleanup_hook/binding.c @@ -0,0 +1,82 @@ +#define NAPI_EXPERIMENTAL +#include "node_api.h" +#include "assert.h" +#include "uv.h" +#include +#include "../../js-native-api/common.h" + +void MustNotCall(void* arg, void(*cb)(void*), void* cbarg) { + 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() { + struct AsyncData* data = (struct AsyncData*) malloc(sizeof(struct AsyncData)); + data->handle = NULL; + return data; +} + +void AfterCleanupHookTwo(uv_handle_t* handle) { + struct AsyncData* data = (struct AsyncData*) handle->data; + data->done_cb(data->done_arg); + 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); + } + + uv_close((uv_handle_t*) async, AfterCleanupHookTwo); +} + +void AsyncCleanupHook(void* arg, void(*cb)(void*), void* cbarg) { + struct AsyncData* data = (struct AsyncData*) arg; + uv_loop_t* loop; + napi_status status = napi_get_uv_event_loop(data->env, &loop); + assert(status == napi_ok); + int err = uv_async_init(loop, &data->async, AfterCleanupHookOne); + 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) { + { + 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); + } + + return NULL; +} + +NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/node-api/test_async_cleanup_hook/binding.gyp b/test/node-api/test_async_cleanup_hook/binding.gyp new file mode 100644 index 00000000000000..23daf507916ff6 --- /dev/null +++ b/test/node-api/test_async_cleanup_hook/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.c' ] + } + ] +} diff --git a/test/node-api/test_async_cleanup_hook/test.js b/test/node-api/test_async_cleanup_hook/test.js new file mode 100644 index 00000000000000..55cc2517a59bc8 --- /dev/null +++ b/test/node-api/test_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)));