From ea73702847b5415df08ee01c92437917cb2c7605 Mon Sep 17 00:00:00 2001 From: ywave620 <60539365+ywave620@users.noreply.github.com> Date: Sun, 25 Dec 2022 17:54:12 +0800 Subject: [PATCH] process,worker: ensure code after exit() effectless Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution() PR-URL: https://github.com/nodejs/node/pull/45620 Reviewed-By: Anna Henningsen Reviewed-By: Yagiz Nizipli Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Minwoo Jung Reviewed-By: Antoine du Hamel --- lib/internal/process/per_thread.js | 12 ++++++++++ src/js_native_api_v8.cc | 3 +++ src/js_native_api_v8.h | 11 +++++++++ src/node_api.cc | 3 +++ 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 +++++++++++++++++++ 10 files changed, 76 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 c2be274659e6c7..18c0abb40df4e4 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -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 { @@ -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) { diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5365c9ceb23a83..157ed6e77fae30 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -314,6 +314,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 b3d0446cb5239f..12b3ab68a8d979 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 c3f78c8b58bc27..49234a23dce800 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -95,6 +95,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) { 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..9f152bd5c62b0c --- /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)); + 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; +} 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..92c4d5596cbddf --- /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)); + 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; + } +}