From f04a702b9c974886b7078062571cd6599c36629d Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 3 Oct 2020 23:29:41 +0200
Subject: [PATCH] src: add maybe versions of EmitExit and EmitBeforeExit
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.
PR-URL: https://github.com/nodejs/node/pull/35486
Backport-PR-URL: https://github.com/nodejs/node/pull/38786
Reviewed-By: James M Snell <jasnell@gmail.com>
---
doc/api/embedding.md | 11 ++--
src/api/hooks.cc | 55 ++++++++++++-------
src/node.h | 15 ++++-
src/node_main_instance.cc | 5 +-
src/node_worker.cc | 5 +-
test/embedding/embedtest.cc | 6 +-
.../test-process-beforeexit-throw-exit.js | 12 ++++
.../test-worker-beforeexit-throw-exit.js | 28 ++++++++++
8 files changed, 105 insertions(+), 32 deletions(-)
create mode 100644 test/parallel/test-process-beforeexit-throw-exit.js
create mode 100644 test/parallel/test-worker-beforeexit-throw-exit.js
diff --git a/doc/api/embedding.md b/doc/api/embedding.md
index 7ec22fbab63850..7dc4a2db7fca63 100644
--- a/doc/api/embedding.md
+++ b/doc/api/embedding.md
@@ -181,9 +181,10 @@ 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.
@@ -191,8 +192,8 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
} 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 --git a/src/api/hooks.cc b/src/api/hooks.cc
index a719a861dbe9d8..84c91a2100b156 100644
--- a/src/api/hooks.cc
+++ b/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 --git a/src/node.h b/src/node.h
index 38e0ef50f9b283..b3412d151b4819 100644
--- a/src/node.h
+++ b/src/node.h
@@ -539,8 +539,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 --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index 0678d1a748edf6..280ccaab5ef89c 100644
--- a/src/node_main_instance.cc
+++ b/src/node_main_instance.cc
@@ -135,7 +135,8 @@ int NodeMainInstance::Run() {
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
@@ -148,7 +149,7 @@ int NodeMainInstance::Run() {
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 --git a/src/node_worker.cc b/src/node_worker.cc
index e2f70f6b357766..8058c4e9caf3da 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -352,7 +352,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.
@@ -368,7 +369,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 --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc
index 21baadf93e5a26..fece8924ad6471 100644
--- a/test/embedding/embedtest.cc
+++ b/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 --git a/test/parallel/test-process-beforeexit-throw-exit.js b/test/parallel/test-process-beforeexit-throw-exit.js
new file mode 100644
index 00000000000000..6e9d764be90baa
--- /dev/null
+++ b/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 --git a/test/parallel/test-worker-beforeexit-throw-exit.js b/test/parallel/test-worker-beforeexit-throw-exit.js
new file mode 100644
index 00000000000000..2aa255ee82af8a
--- /dev/null
+++ b/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);
+}));