Skip to content

Commit

Permalink
process,worker: ensure code after exit() effectless
Browse files Browse the repository at this point in the history
Cope with the delay(to the next function call) of
v8::Isolate::TerminateExecution()

PR-URL: #45620
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
  • Loading branch information
ywave620 authored and juanarbol committed Jan 31, 2023
1 parent 16ee02f commit ea73702
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 5 deletions.
12 changes: 12 additions & 0 deletions lib/internal/process/per_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ function hrtimeBigInt() {
return hrBigintValues[0];
}

function nop() {}

// The execution of this function itself should not cause any side effects.
function wrapProcessMethods(binding) {
const {
Expand Down Expand Up @@ -193,6 +195,16 @@ function wrapProcessMethods(binding) {
// in the user land. Either document it, or deprecate it in favor of a
// better public alternative.
process.reallyExit(process.exitCode || 0);

// If this is a worker, v8::Isolate::TerminateExecution() is called above.
// That function spoofs the stack pointer to cause the stack guard
// check to throw the termination exception. Because v8 performs
// stack guard check upon every function call, we give it a chance.
//
// Without this, user code after `process.exit()` would take effect.
// test/parallel/test-worker-voluntarily-exit-followed-by-addition.js
// test/parallel/test-worker-voluntarily-exit-followed-by-throw.js
nop();
}

function kill(pid, sig) {
Expand Down
3 changes: 3 additions & 0 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ class CallbackWrapperBase : public CallbackWrapper {
env->CallIntoModule([&](napi_env env) { result = cb(env, cbinfo_wrapper); },
[&](napi_env env, v8::Local<v8::Value> value) {
exceptionOccurred = true;
if (env->terminatedOrTerminating()) {
return;
}
env->isolate->ThrowException(value);
});

Expand Down
11 changes: 11 additions & 0 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,20 @@ struct napi_env__ {
}

static inline void HandleThrow(napi_env env, v8::Local<v8::Value> value) {
if (env->terminatedOrTerminating()) {
return;
}
env->isolate->ThrowException(value);
}

// i.e. whether v8 exited or is about to exit
inline bool terminatedOrTerminating() {
return this->isolate->IsExecutionTerminating() || !can_call_into_js();
}

// v8 uses a special exception to indicate termination, the
// `handle_exception` callback should identify such case using
// terminatedOrTerminating() before actually handle the exception
template <typename T, typename U = decltype(HandleThrow)>
inline void CallIntoModule(T&& call, U&& handle_exception = HandleThrow) {
int open_handle_scopes_before = open_handle_scopes;
Expand Down
3 changes: 3 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ template <bool enforceUncaughtExceptionPolicy, typename T>
void node_napi_env__::CallbackIntoModule(T&& call) {
CallIntoModule(call, [](napi_env env_, v8::Local<v8::Value> local_err) {
node_napi_env__* env = static_cast<node_napi_env__*>(env_);
if (env->terminatedOrTerminating()) {
return;
}
node::Environment* node_env = env->node_env();
if (!node_env->options()->force_node_api_uncaught_exceptions_policy &&
!enforceUncaughtExceptionPolicy) {
Expand Down
4 changes: 3 additions & 1 deletion test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,9 @@ TEST_F(EnvironmentTest, ExitHandlerTest) {
callback_calls++;
node::Stop(*env);
});
node::LoadEnvironment(*env, "process.exit(42)").ToLocalChecked();
// When terminating, v8 throws makes the current embedder call bail out
// with MaybeLocal<>()
EXPECT_TRUE(node::LoadEnvironment(*env, "process.exit(42)").IsEmpty());
EXPECT_EQ(callback_calls, 1);
}

Expand Down
4 changes: 2 additions & 2 deletions test/node-api/test_worker_terminate/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ if (isMainThread) {
const { Test } = require(`./build/${common.buildType}/test_worker_terminate`);

const { counter } = workerData;
// Test() tries to call a function twice and asserts that the second call does
// not work because of a pending exception.
// Test() tries to call a function and asserts it fails because of a
// pending termination exception.
Test(() => {
Atomics.add(counter, 0, 1);
process.exit();
Expand Down
2 changes: 0 additions & 2 deletions test/node-api/test_worker_terminate/test_worker_terminate.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ napi_value Test(napi_env env, napi_callback_info info) {
NODE_API_ASSERT(env, t == napi_function,
"Wrong first argument, function expected.");

status = napi_call_function(env, recv, argv[0], 0, NULL, NULL);
assert(status == napi_ok);
status = napi_call_function(env, recv, argv[0], 0, NULL, NULL);
assert(status == napi_pending_exception);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const { Worker } = require('worker_threads');
const workerData = new Int32Array(new SharedArrayBuffer(4));
const w = new Worker(`
const { createHook } = require('async_hooks');
const { workerData } = require('worker_threads');
setImmediate(async () => {
createHook({ init() {} }).enable();
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-worker-voluntarily-exit-followed-by-addition.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { Worker, isMainThread } = require('worker_threads');

if (isMainThread) {
const workerData = new Int32Array(new SharedArrayBuffer(4));
new Worker(__filename, {
workerData,
});
process.on('beforeExit', common.mustCall(() => {
assert.strictEqual(workerData[0], 0);
}));
} else {
const { workerData } = require('worker_threads');
process.exit();
workerData[0] = 1;
}
23 changes: 23 additions & 0 deletions test/parallel/test-worker-voluntarily-exit-followed-by-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { Worker, isMainThread } = require('worker_threads');

if (isMainThread) {
const workerData = new Int32Array(new SharedArrayBuffer(4));
new Worker(__filename, {
workerData,
});
process.on('beforeExit', common.mustCall(() => {
assert.strictEqual(workerData[0], 0);
}));
} else {
const { workerData } = require('worker_threads');
try {
process.exit();
throw new Error('xxx');
// eslint-disable-next-line no-unused-vars
} catch (err) {
workerData[0] = 1;
}
}

0 comments on commit ea73702

Please sign in to comment.