Skip to content

Commit

Permalink
src: add maybe versions of EmitExit and EmitBeforeExit
Browse files Browse the repository at this point in the history
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
addaleax authored and MylesBorins committed Aug 31, 2021
1 parent 7031eae commit f421422
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 32 deletions.
11 changes: 6 additions & 5 deletions doc/api/embedding.md
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
55 changes: 36 additions & 19 deletions src/api/hooks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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())
Expand All @@ -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);
Expand Down
15 changes: 13 additions & 2 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
5 changes: 3 additions & 2 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand Down
6 changes: 4 additions & 2 deletions test/embedding/embedtest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-process-beforeexit-throw-exit.js
Original file line number Diff line number Diff line change
@@ -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();
}));
28 changes: 28 additions & 0 deletions test/parallel/test-worker-beforeexit-throw-exit.js
Original file line number Diff line number Diff line change
@@ -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);
}));

0 comments on commit f421422

Please sign in to comment.