src: add maybe versions of EmitExit and EmitBeforeExit

This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

Diff for: doc/api/embedding.md

@@ -181,18 +181,19 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
         more = uv_loop_alive(&loop);
         if (more) continue;
 
-        // node::EmitBeforeExit() is used to emit the 'beforeExit' event on
-        // the `process` object.
-        node::EmitBeforeExit(env.get());
+        // node::EmitProcessBeforeExit() is used to emit the 'beforeExit' event
+        // on the `process` object.
+        if (node::EmitProcessBeforeExit(env.get()).IsNothing())
+          break;
 
         // 'beforeExit' can also schedule new work that keeps the event loop
         // running.
         more = uv_loop_alive(&loop);
       } while (more == true);
     }
 
-    // node::EmitExit() returns the current exit code.
-    exit_code = node::EmitExit(env.get());
+    // node::EmitProcessExit() returns the current exit code.
+    exit_code = node::EmitProcessExit(env.get()).FromMaybe(1);
 
     // node::Stop() can be used to explicitly stop the event loop and keep
     // further JavaScript from running. It can be called from any thread,

Diff for: src/api/hooks.cc

@@ -9,8 +9,11 @@ using v8::Context;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
+using v8::Maybe;
 using v8::NewStringType;
+using v8::Nothing;
 using v8::Object;
 using v8::String;
 using v8::Value;
@@ -30,6 +33,10 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) {
 }
 
 void EmitBeforeExit(Environment* env) {
+  USE(EmitProcessBeforeExit(env));
+}
+
+Maybe<bool> EmitProcessBeforeExit(Environment* env) {
   TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
                               "BeforeExit", env);
   if (!env->destroy_async_id_list()->empty())
@@ -40,39 +47,49 @@ void EmitBeforeExit(Environment* env) {
 
   Local<Value> exit_code_v;
   if (!env->process_object()->Get(env->context(), env->exit_code_string())
-      .ToLocal(&exit_code_v)) return;
+      .ToLocal(&exit_code_v)) return Nothing<bool>();
 
   Local<Integer> exit_code;
-  if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) return;
+  if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) {
+    return Nothing<bool>();
+  }
 
-  // TODO(addaleax): Provide variants of EmitExit() and EmitBeforeExit() that
-  // actually forward empty MaybeLocal<>s (and check env->can_call_into_js()).
-  USE(ProcessEmit(env, "beforeExit", exit_code));
+  return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ?
+      Nothing<bool>() : Just(true);
 }
 
 int EmitExit(Environment* env) {
+  return EmitProcessExit(env).FromMaybe(1);
+}
+
+Maybe<int> EmitProcessExit(Environment* env) {
   // process.emit('exit')
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
   Local<Object> process_object = env->process_object();
-  process_object
+
+  // TODO(addaleax): It might be nice to share process._exiting and
+  // process.exitCode via getter/setter pairs that pass data directly to the
+  // native side, so that we don't manually have to read and write JS properties
+  // here. These getters could use e.g. a typed array for performance.
+  if (process_object
       ->Set(env->context(),
             FIXED_ONE_BYTE_STRING(env->isolate(), "_exiting"),
-            True(env->isolate()))
-      .Check();
+            True(env->isolate())).IsNothing()) return Nothing<int>();
 
   Local<String> exit_code = env->exit_code_string();
-  int code = process_object->Get(env->context(), exit_code)
-                 .ToLocalChecked()
-                 ->Int32Value(env->context())
-                 .ToChecked();
-  ProcessEmit(env, "exit", Integer::New(env->isolate(), code));
-
-  // Reload exit code, it may be changed by `emit('exit')`
-  return process_object->Get(env->context(), exit_code)
-      .ToLocalChecked()
-      ->Int32Value(env->context())
-      .ToChecked();
+  Local<Value> code_v;
+  int code;
+  if (!process_object->Get(env->context(), exit_code).ToLocal(&code_v) ||
+      !code_v->Int32Value(env->context()).To(&code) ||
+      ProcessEmit(env, "exit", Integer::New(env->isolate(), code)).IsEmpty() ||
+      // Reload exit code, it may be changed by `emit('exit')`
+      !process_object->Get(env->context(), exit_code).ToLocal(&code_v) ||
+      !code_v->Int32Value(env->context()).To(&code)) {
+    return Nothing<int>();
+  }
+
+  return Just(code);
 }
 
 typedef void (*CleanupHook)(void* arg);

Diff for: src/node.h

@@ -534,8 +534,19 @@ NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform);
 NODE_EXTERN v8::TracingController* GetTracingController();
 NODE_EXTERN void SetTracingController(v8::TracingController* controller);
 
-NODE_EXTERN void EmitBeforeExit(Environment* env);
-NODE_EXTERN int EmitExit(Environment* env);
+// Run `process.emit('beforeExit')` as it would usually happen when Node.js is
+// run in standalone mode.
+NODE_EXTERN v8::Maybe<bool> EmitProcessBeforeExit(Environment* env);
+NODE_DEPRECATED("Use Maybe version (EmitProcessBeforeExit) instead",
+    NODE_EXTERN void EmitBeforeExit(Environment* env));
+// Run `process.emit('exit')` as it would usually happen when Node.js is run
+// in standalone mode. The return value corresponds to the exit code.
+NODE_EXTERN v8::Maybe<int> EmitProcessExit(Environment* env);
+NODE_DEPRECATED("Use Maybe version (EmitProcessExit) instead",
+    NODE_EXTERN int EmitExit(Environment* env));
+
+// Runs hooks added through `AtExit()`. This is part of `FreeEnvironment()`,
+// so calling it manually is typically not necessary.
 NODE_EXTERN void RunAtExit(Environment* env);
 
 // This may return nullptr if the current v8::Context is not associated

Diff for: src/node_main_instance.cc

@@ -156,7 +156,8 @@ int NodeMainInstance::Run(const EnvSerializeInfo* env_info) {
           if (more && !env->is_stopping()) continue;
 
           if (!uv_loop_alive(env->event_loop())) {
-            EmitBeforeExit(env.get());
+            if (EmitProcessBeforeExit(env.get()).IsNothing())
+              break;
           }
 
           // Emit `beforeExit` if the loop became alive either after emitting
@@ -169,7 +170,7 @@ int NodeMainInstance::Run(const EnvSerializeInfo* env_info) {
 
       env->set_trace_sync_io(false);
       if (!env->is_stopping()) env->VerifyNoStrongBaseObjects();
-      exit_code = EmitExit(env.get());
+      exit_code = EmitProcessExit(env.get()).FromMaybe(1);
     }
 
     ResetStdio();

Diff for: src/node_worker.cc

@@ -348,7 +348,8 @@ void Worker::Run() {
           more = uv_loop_alive(&data.loop_);
           if (more && !is_stopped()) continue;
 
-          EmitBeforeExit(env_.get());
+          if (EmitProcessBeforeExit(env_.get()).IsNothing())
+            break;
 
           // Emit `beforeExit` if the loop became alive either after emitting
           // event, or after running some callbacks.
@@ -364,7 +365,7 @@ void Worker::Run() {
       bool stopped = is_stopped();
       if (!stopped) {
         env_->VerifyNoStrongBaseObjects();
-        exit_code = EmitExit(env_.get());
+        exit_code = EmitProcessExit(env_.get()).FromMaybe(1);
       }
       Mutex::ScopedLock lock(mutex_);
       if (exit_code_ == 0 && !stopped)

Diff for: test/embedding/embedtest.cc

@@ -110,12 +110,14 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
         more = uv_loop_alive(&loop);
         if (more) continue;
 
-        node::EmitBeforeExit(env.get());
+        if (node::EmitProcessBeforeExit(env.get()).IsNothing())
+          break;
+
         more = uv_loop_alive(&loop);
       } while (more == true);
     }
 
-    exit_code = node::EmitExit(env.get());
+    exit_code = node::EmitProcessExit(env.get()).FromMaybe(1);
 
     node::Stop(env.get());
   }

Diff for: test/parallel/test-process-beforeexit-throw-exit.js

@@ -0,0 +1,12 @@
+'use strict';
+const common = require('../common');
+common.skipIfWorker();
+
+// Test that 'exit' is emitted if 'beforeExit' throws.
+
+process.on('exit', common.mustCall(() => {
+  process.exitCode = 0;
+}));
+process.on('beforeExit', common.mustCall(() => {
+  throw new Error();
+}));

Diff for: test/parallel/test-worker-beforeexit-throw-exit.js

@@ -0,0 +1,28 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+// Test that 'exit' is emitted if 'beforeExit' throws, both inside the Worker.
+
+const workerData = new Uint8Array(new SharedArrayBuffer(2));
+const w = new Worker(`
+  const { workerData } = require('worker_threads');
+  process.on('exit', () => {
+    workerData[0] = 100;
+  });
+  process.on('beforeExit', () => {
+    workerData[1] = 200;
+    throw new Error('banana');
+  });
+`, { eval: true, workerData });
+
+w.on('error', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'banana');
+}));
+
+w.on('exit', common.mustCall((code) => {
+  assert.strictEqual(code, 1);
+  assert.strictEqual(workerData[0], 100);
+  assert.strictEqual(workerData[1], 200);
+}));