diff --git a/src/api/callback.cc b/src/api/callback.cc index 1287eb466fddd3..3a8bd9155a8545 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -97,10 +97,9 @@ void InternalCallbackScope::Close() { if (closed_) return; closed_ = true; - Isolate* isolate = env_->isolate(); - auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); }); + // This function must ends up with either cleanup the + // async id stack or pop the topmost one from it - if (!env_->can_call_into_js()) return; auto perform_stopping_check = [&]() { if (env_->is_stopping()) { MarkAsFailed(); @@ -109,6 +108,11 @@ void InternalCallbackScope::Close() { }; perform_stopping_check(); + if (env_->is_stopping()) return; + + Isolate* isolate = env_->isolate(); + auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); }); + if (!failed_ && async_context_.async_id != 0 && !skip_hooks_) { AsyncWrap::EmitAfter(env_, async_context_.async_id); } diff --git a/src/api/environment.cc b/src/api/environment.cc index 389c6666dc6966..19c50398fa3da5 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -881,6 +881,7 @@ ThreadId AllocateEnvironmentThreadId() { } void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code) { + env->set_stopping(true); env->set_can_call_into_js(false); env->stop_sub_worker_contexts(); env->isolate()->DumpAndResetStats(); diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js new file mode 100644 index 00000000000000..a3e823ca70bf0f --- /dev/null +++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js @@ -0,0 +1,32 @@ +'use strict'; +const common = require('../common'); + +// https://github.com/nodejs/node/issues/45421 +// +// Check that node will NOT call v8::Isolate::SetIdle() when exiting +// due to an unhandled exception, otherwise the assertion(enabled in +// debug build only) in the SetIdle(), which checks that the vm state +// is either EXTERNAL or IDLE will fail. +// +// The root cause of this issue is that before PerIsolateMessageListener() +// is invoked by v8, v8 preserves the JS vm state, although it should +// switch to EXTERNEL. https://bugs.chromium.org/p/v8/issues/detail?id=13464 +// +// Therefore, this commit can be considered as an workaround of the v8 bug, +// but we also find it not useful to call SetIdle() when terminating. + +if (process.argv[2] === 'child') { + const { Worker } = require('worker_threads'); + new Worker('', { eval: true }); + throw new Error('xxx'); +} else { + const assert = require('assert'); + const { spawnSync } = require('child_process'); + const result = spawnSync(process.execPath, [__filename, 'child']); + + const stderr = result.stderr.toString().trim(); + // Expect error message to be preserved + assert.match(stderr, /xxx/); + // Expect no crash + assert(!common.nodeProcessAborted(result.status, result.signal), stderr); +} diff --git a/test/sequential/test-worker-http2-stream-terminate.js b/test/parallel/test-worker-http2-stream-terminate.js similarity index 100% rename from test/sequential/test-worker-http2-stream-terminate.js rename to test/parallel/test-worker-http2-stream-terminate.js