From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 17 Aug 2020 10:13:00 -0700 Subject: 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 Co-authored-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/34819 Reviewed-By: Michael Dawson Reviewed-By: Zeyu Yang diff --git a/doc/api/n-api.md b/doc/api/n-api.md index d9b757721c880388664f11cc5c0425379e105e2b..161f61308e5e5faa1ddf68802fdafacf1e5c1613 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 + + +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 + + +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 > 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 > 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 @@ -5757,6 +5807,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 4fbab771d5840004a303094c87981409d8bac848..93488146d56690c27c56a21f2795796d027cfa02 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(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(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(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 4f3eb8f2caae6375d5334486d75be76bf912d4e3..577a1dcd94987202819e7a36a2d9674f13d13614 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 b8711d3eddc408bc239a964528c23d71555a5d72..0e400e9676df5ba09d350fe7a2a70a1dc9e4d3d6 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 f0c9cd97a26c48c3f7323930dc856e49e1755f35..7bbde56bb0ec888a97926f36425f7a1dca719514 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 #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;