From de62467497bd242d20ae8d543c7cea58faf03ef5 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Fri, 25 Nov 2022 15:17:41 +0800 Subject: [PATCH 1/5] process,worker: ensure code after exit() effectless Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution() --- lib/internal/process/per_thread.js | 12 ++++++++++ test/cctest/test_environment.cc | 4 +++- test/node-api/test_worker_terminate/test.js | 4 ++-- .../test_worker_terminate.c | 2 -- ...-async-hooks-worker-asyncfn-terminate-4.js | 1 + ...r-voluntarily-exit-followed-by-addition.js | 18 +++++++++++++++ ...rker-voluntarily-exit-followed-by-throw.js | 23 +++++++++++++++++++ 7 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-worker-voluntarily-exit-followed-by-addition.js create mode 100644 test/parallel/test-worker-voluntarily-exit-followed-by-throw.js diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index b2a15a46484255..ed1b82d13e77cc 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -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 { @@ -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 followed by `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) { diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 812962cd5c1a71..547c8ddbffe243 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -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); } diff --git a/test/node-api/test_worker_terminate/test.js b/test/node-api/test_worker_terminate/test.js index 7c7224c073dda3..eefb974af5a669 100644 --- a/test/node-api/test_worker_terminate/test.js +++ b/test/node-api/test_worker_terminate/test.js @@ -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(); diff --git a/test/node-api/test_worker_terminate/test_worker_terminate.c b/test/node-api/test_worker_terminate/test_worker_terminate.c index 3c428195b9a571..48e3e0fa675ed3 100644 --- a/test/node-api/test_worker_terminate/test_worker_terminate.c +++ b/test/node-api/test_worker_terminate/test_worker_terminate.c @@ -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); diff --git a/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js b/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js index dc641471b691e0..c522091006cf5a 100644 --- a/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js +++ b/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js @@ -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(); diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js b/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js new file mode 100644 index 00000000000000..9bb706bdd8d4c7 --- /dev/null +++ b/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)); + const w = new Worker(__filename, { + workerData, + }); + w.on('exit', common.mustCall(() => { + assert.strictEqual(workerData[0], 0); + })); +} else { + const { workerData } = require('worker_threads'); + process.exit(); + workerData[0] = 1; +} diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js b/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js new file mode 100644 index 00000000000000..d5a599c2bed5bb --- /dev/null +++ b/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)); + const w = new Worker(__filename, { + workerData, + }); + w.on('exit', 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; + } +} From 37cccd035ae6fc786db41b9bc53b3ad7eb703463 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Fri, 2 Dec 2022 09:52:56 +0800 Subject: [PATCH 2/5] process,worker: ensure code after exit() effectless fix typo --- lib/internal/process/per_thread.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index ed1b82d13e77cc..c162d37150ed39 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -203,7 +203,7 @@ function wrapProcessMethods(binding) { // 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 followed by `process.exit()` would take effect. + // 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(); From 0f66f9ea32e163a29f9d241d37d624fc92b0f1af Mon Sep 17 00:00:00 2001 From: ywave620 Date: Sat, 3 Dec 2022 10:52:00 +0800 Subject: [PATCH 3/5] process,worker: ensure code after exit() effectless check the worker exits properly in main thread's beforeExit hook --- .../test-worker-voluntarily-exit-followed-by-addition.js | 2 +- test/parallel/test-worker-voluntarily-exit-followed-by-throw.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js b/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js index 9bb706bdd8d4c7..bb58f8fe7a3ffb 100644 --- a/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js +++ b/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js @@ -8,7 +8,7 @@ if (isMainThread) { const w = new Worker(__filename, { workerData, }); - w.on('exit', common.mustCall(() => { + process.on('beforeExit', common.mustCall(() => { assert.strictEqual(workerData[0], 0); })); } else { diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js b/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js index d5a599c2bed5bb..f66d65dcee014e 100644 --- a/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js +++ b/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js @@ -8,7 +8,7 @@ if (isMainThread) { const w = new Worker(__filename, { workerData, }); - w.on('exit', common.mustCall(() => { + w.on('beforeExit', common.mustCall(() => { assert.strictEqual(workerData[0], 0); })); } else { From ad562e15c39aa123b2d2efd7b3e0b3c19bbd654c Mon Sep 17 00:00:00 2001 From: ywave620 Date: Sat, 3 Dec 2022 11:04:14 +0800 Subject: [PATCH 4/5] process,worker: ensure code after exit() effectless check the worker exits properly in main thread's beforeExit hook. fix lint and typo. --- .../test-worker-voluntarily-exit-followed-by-addition.js | 2 +- .../test-worker-voluntarily-exit-followed-by-throw.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js b/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js index bb58f8fe7a3ffb..9f152bd5c62b0c 100644 --- a/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js +++ b/test/parallel/test-worker-voluntarily-exit-followed-by-addition.js @@ -5,7 +5,7 @@ const { Worker, isMainThread } = require('worker_threads'); if (isMainThread) { const workerData = new Int32Array(new SharedArrayBuffer(4)); - const w = new Worker(__filename, { + new Worker(__filename, { workerData, }); process.on('beforeExit', common.mustCall(() => { diff --git a/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js b/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js index f66d65dcee014e..92c4d5596cbddf 100644 --- a/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js +++ b/test/parallel/test-worker-voluntarily-exit-followed-by-throw.js @@ -5,10 +5,10 @@ const { Worker, isMainThread } = require('worker_threads'); if (isMainThread) { const workerData = new Int32Array(new SharedArrayBuffer(4)); - const w = new Worker(__filename, { + new Worker(__filename, { workerData, }); - w.on('beforeExit', common.mustCall(() => { + process.on('beforeExit', common.mustCall(() => { assert.strictEqual(workerData[0], 0); })); } else { From 0c912206da368ee642ca5172ea1f3a4cd76b9102 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Sat, 10 Dec 2022 19:22:58 +0800 Subject: [PATCH 5/5] process,worker: ensure code after exit() effectless ignore exception that indicaties a termination in napi call --- src/js_native_api_v8.cc | 3 +++ src/js_native_api_v8.h | 11 +++++++++++ src/node_api.cc | 3 +++ 3 files changed, 17 insertions(+) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index abc9e56d6b0bbb..b96f8da85002f7 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -308,6 +308,9 @@ class CallbackWrapperBase : public CallbackWrapper { env->CallIntoModule([&](napi_env env) { result = cb(env, cbinfo_wrapper); }, [&](napi_env env, v8::Local value) { exceptionOccurred = true; + if (env->terminatedOrTerminating()) { + return; + } env->isolate->ThrowException(value); }); diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 766398744c5dfb..c4fd883132822d 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -72,9 +72,20 @@ struct napi_env__ { } static inline void HandleThrow(napi_env env, v8::Local 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 inline void CallIntoModule(T&& call, U&& handle_exception = HandleThrow) { int open_handle_scopes_before = open_handle_scopes; diff --git a/src/node_api.cc b/src/node_api.cc index 8f1e4fb0e40d4a..08373dcabb1f40 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -82,6 +82,9 @@ template void node_napi_env__::CallbackIntoModule(T&& call) { CallIntoModule(call, [](napi_env env_, v8::Local local_err) { node_napi_env__* env = static_cast(env_); + if (env->terminatedOrTerminating()) { + return; + } node::Environment* node_env = env->node_env(); if (!node_env->options()->force_node_api_uncaught_exceptions_policy && !enforceUncaughtExceptionPolicy) {