From cd6d6215ccb15bfe8491267033b2b86abfde212c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 20:41:48 +0100 Subject: [PATCH] src: deprecate two- and one-argument AtExit() Using `AtExit()` without an `Environment*` pointer or providing an argument is almost always a sign of improperly relying on global state and/or using `AtExit()` as an addon when the addon-targeting `AddEnvironmentCleanupHook()` would be the better choice. Deprecate those variants. This also updates the addon docs to refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`. PR-URL: https://github.com/nodejs/node/pull/30227 Reviewed-By: David Carlier Reviewed-By: Franziska Hinkelmann Reviewed-By: Daniel Bevenius Reviewed-By: James M Snell --- doc/api/addons.md | 144 ++++++++++++++------------------- src/node.h | 15 +++- test/addons/at-exit/binding.cc | 11 ++- 3 files changed, 81 insertions(+), 89 deletions(-) diff --git a/doc/api/addons.md b/doc/api/addons.md index 08475daa33a420..6e5e9bf18c8418 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -241,6 +241,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: + +* Be an N-API addon, or +* Be declared as context-aware using `NODE_MODULE_INIT()` as described above + In order to support [`Worker`][] threads, addons need to clean up any resources they may have allocated when such a thread exists. This can be achieved through the usage of the `AddEnvironmentCleanupHook()` function: @@ -254,13 +260,62 @@ void AddEnvironmentCleanupHook(v8::Isolate* isolate, This function adds a hook that will run before a given Node.js instance shuts down. If necessary, such hooks can be removed using `RemoveEnvironmentCleanupHook()` before they are run, which has the same -signature. +signature. Callbacks are run in last-in first-out order. -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: +The following `addon.cc` uses `AddEnvironmentCleanupHook`: -* Be an N-API addon, or -* Be declared as context-aware using `NODE_MODULE_INIT()` as described above +```cpp +// addon.cc +#include +#include +#include + +using node::AddEnvironmentCleanupHook; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Object; + +// Note: In a real-world application, do not rely on static/global data. +static char cookie[] = "yum yum"; +static int cleanup_cb1_called = 0; +static int cleanup_cb2_called = 0; + +static void cleanup_cb1(void* arg) { + Isolate* isolate = static_cast(arg); + HandleScope scope(isolate); + Local obj = Object::New(isolate); + assert(!obj.IsEmpty()); // assert VM is still alive + assert(obj->IsObject()); + cleanup_cb1_called++; +} + +static void cleanup_cb2(void* arg) { + assert(arg == static_cast(cookie)); + cleanup_cb2_called++; +} + +static void sanity_check(void*) { + assert(cleanup_cb1_called == 1); + assert(cleanup_cb2_called == 1); +} + +// Initialize this addon to be context-aware. +NODE_MODULE_INIT(/* exports, module, context */) { + Isolate* isolate = context->GetIsolate(); + + AddEnvironmentCleanupHook(isolate, sanity_check, nullptr); + AddEnvironmentCleanupHook(isolate, cleanup_cb2, cookie); + AddEnvironmentCleanupHook(isolate, cleanup_cb1, isolate); +} +``` + +Test in JavaScript by running: + +```js +// test.js +require('./build/Release/addon'); +``` ### Building @@ -1293,85 +1348,6 @@ console.log(result); // Prints: 30 ``` -### AtExit hooks - -An `AtExit` hook is a function that is invoked after the Node.js event loop -has ended but before the JavaScript VM is terminated and Node.js shuts down. -`AtExit` hooks are registered using the `node::AtExit` API. - -#### void AtExit(callback, args) - -* `callback` <void (\*)(void\*)> - A pointer to the function to call at exit. -* `args` <void\*> - A pointer to pass to the callback at exit. - -Registers exit hooks that run after the event loop has ended but before the VM -is killed. - -`AtExit` takes two parameters: a pointer to a callback function to run at exit, -and a pointer to untyped context data to be passed to that callback. - -Callbacks are run in last-in first-out order. - -The following `addon.cc` implements `AtExit`: - -```cpp -// addon.cc -#include -#include -#include - -namespace demo { - -using node::AtExit; -using v8::HandleScope; -using v8::Isolate; -using v8::Local; -using v8::Object; - -static char cookie[] = "yum yum"; -static int at_exit_cb1_called = 0; -static int at_exit_cb2_called = 0; - -static void at_exit_cb1(void* arg) { - Isolate* isolate = static_cast(arg); - HandleScope scope(isolate); - Local obj = Object::New(isolate); - assert(!obj.IsEmpty()); // assert VM is still alive - assert(obj->IsObject()); - at_exit_cb1_called++; -} - -static void at_exit_cb2(void* arg) { - assert(arg == static_cast(cookie)); - at_exit_cb2_called++; -} - -static void sanity_check(void*) { - assert(at_exit_cb1_called == 1); - assert(at_exit_cb2_called == 2); -} - -void init(Local exports) { - AtExit(sanity_check); - AtExit(at_exit_cb2, cookie); - AtExit(at_exit_cb2, cookie); - AtExit(at_exit_cb1, exports->GetIsolate()); -} - -NODE_MODULE(NODE_GYP_MODULE_NAME, init) - -} // namespace demo -``` - -Test in JavaScript by running: - -```js -// test.js -require('./build/Release/addon'); -``` - [`Worker`]: worker_threads.html#worker_threads_class_worker [Electron]: https://electronjs.org/ [Embedder's Guide]: https://github.com/v8/v8/wiki/Embedder's%20Guide diff --git a/src/node.h b/src/node.h index ea5d8a20a41593..8215552227a2aa 100644 --- a/src/node.h +++ b/src/node.h @@ -670,8 +670,13 @@ extern "C" NODE_EXTERN void node_module_register(void* mod); /* Called after the event loop exits but before the VM is disposed. * Callbacks are run in reverse order of registration, i.e. newest first. + * + * You should always use the three-argument variant (or, for addons, + * AddEnvironmentCleanupHook) in order to avoid relying on global state. */ -NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = nullptr); +NODE_DEPRECATED( + "Use the three-argument variant of AtExit() or AddEnvironmentCleanupHook()", + NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = nullptr)); /* Registers a callback with the passed-in Environment instance. The callback * is called after the event loop exits, but before the VM is disposed. @@ -679,7 +684,13 @@ NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = nullptr); */ NODE_EXTERN void AtExit(Environment* env, void (*cb)(void* arg), - void* arg = nullptr); + void* arg); +NODE_DEPRECATED( + "Use the three-argument variant of AtExit() or AddEnvironmentCleanupHook()", + inline void AtExit(Environment* env, + void (*cb)(void* arg)) { + AtExit(env, cb, nullptr); + }) typedef double async_id; struct async_context { diff --git a/test/addons/at-exit/binding.cc b/test/addons/at-exit/binding.cc index 3a27bcd7c30756..b091c65ed91eeb 100644 --- a/test/addons/at-exit/binding.cc +++ b/test/addons/at-exit/binding.cc @@ -3,7 +3,11 @@ #include #include +// TODO(addaleax): Maybe merge this file with the cctest for AtExit()? + using node::AtExit; +using node::Environment; +using node::GetCurrentEnvironment; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -46,9 +50,10 @@ NODE_C_DTOR(sanity_check) { } void init(Local exports) { - AtExit(at_exit_cb1, exports->GetIsolate()); - AtExit(at_exit_cb2, cookie); - AtExit(at_exit_cb2, cookie); + Environment* env = GetCurrentEnvironment(exports->CreationContext()); + AtExit(env, at_exit_cb1, exports->GetIsolate()); + AtExit(env, at_exit_cb2, cookie); + AtExit(env, at_exit_cb2, cookie); } NODE_MODULE(NODE_GYP_MODULE_NAME, init)