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: #30227
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <assert.h>
+#include <stdlib.h>
+#include <node.h>
+
+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<Isolate*>(arg);
+  HandleScope scope(isolate);
+  Local<Object> 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<void*>(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` <span class="type">&lt;void (\*)(void\*)&gt;</span>
-  A pointer to the function to call at exit.
-* `args` <span class="type">&lt;void\*&gt;</span>
-  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 <assert.h>
-#include <stdlib.h>
-#include <node.h>
-
-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<Isolate*>(arg);
-  HandleScope scope(isolate);
-  Local<Object> 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<void*>(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<Object> 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

src/node.h

@@ -664,16 +664,27 @@ 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.
  * Callbacks are run in reverse order of registration, i.e. newest first.
  */
 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 {

test/addons/at-exit/binding.cc

@@ -3,7 +3,11 @@
 #include <node.h>
 #include <v8.h>
 
+// 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<Object> 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)