From 6c2f31a1f290b7bd887cb4f26a777ef96dca3456 Mon Sep 17 00:00:00 2001 From: Anna Henningsen 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 --- 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 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 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(); Local 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(); + } - // 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() : Just(true); } int EmitExit(Environment* env) { + return EmitProcessExit(env).FromMaybe(1); +} + +Maybe EmitProcessExit(Environment* env) { // process.emit('exit') HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local 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(); Local 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 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(); + } + + 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 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 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); +}));