Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process,worker: ensure code after exit() effectless #45620

Merged
merged 5 commits into from Dec 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions lib/internal/process/per_thread.js
Expand Up @@ -104,6 +104,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 @@ -195,6 +197,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 || kNoFailure);

// 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
Expand Up @@ -308,6 +308,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
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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if this->isolate->IsExecutionTerminating() is true, we can not call can_call_into_js(), because that will extract the node::Environment by calling v8 API on this->isolate , which will again hit the assertion:

# Fatal error in ../deps/v8/src/api/api.cc, line 8635
# Debug check failed: !i_isolate->is_execution_terminating().

}

// 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
Expand Up @@ -82,6 +82,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
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
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
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
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');
Copy link
Contributor Author

@ywave620 ywave620 Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the original author forgot to add this import. And this is why the strictEqual below paased, because adding to the workerData raises an reference exception.


setImmediate(async () => {
createHook({ init() {} }).enable();
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-worker-voluntarily-exit-followed-by-addition.js
@@ -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
@@ -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) {
ywave620 marked this conversation as resolved.
Show resolved Hide resolved
workerData[0] = 1;
}
}