Skip to content

Commit 53f19ce

Browse files
committed
src: fix cb scope bugs involved in termination
Be more aggresive to clean up the async id stack, and ensure the cleanup when terminating. Do not call SetIdle() when terminating to tolerate https://bugs.chromium.org/p/v8/issues/detail?id=13464
1 parent 2c47493 commit 53f19ce

File tree

3 files changed

+11
-7
lines changed

3 files changed

+11
-7
lines changed

src/api/callback.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,14 +101,14 @@ void InternalCallbackScope::Close() {
101101
// async id stack or pop the topmost one from it
102102

103103
auto perform_stopping_check = [&]() {
104-
if (!env_->can_call_into_js()) {
104+
if (env_->is_stopping()) {
105105
MarkAsFailed();
106106
env_->async_hooks()->clear_async_id_stack();
107107
}
108108
};
109109
perform_stopping_check();
110110

111-
if (!env_->can_call_into_js()) return;
111+
if (env_->is_stopping()) return;
112112

113113
Isolate* isolate = env_->isolate();
114114
auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });

src/api/environment.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -881,6 +881,7 @@ ThreadId AllocateEnvironmentThreadId() {
881881
}
882882

883883
void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code) {
884+
env->set_stopping(true);
884885
env->set_can_call_into_js(false);
885886
env->stop_sub_worker_contexts();
886887
env->isolate()->DumpAndResetStats();

test/parallel/test-unhandled-exception-with-worker-inuse.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,17 @@ const common = require('../common');
33

44
// https://github.com/nodejs/node/issues/45421
55
//
6-
// Check that node will not call v8::Isolate::SetIdle() when exiting
6+
// Check that node will NOT call v8::Isolate::SetIdle() when exiting
77
// due to an unhandled exception, otherwise the assertion(enabled in
8-
// debug build only) in the SetIdle() will fail.
8+
// debug build only) in the SetIdle(), which checks that the vm state
9+
// is either EXTERNAL or IDLE will fail.
910
//
1011
// The root cause of this issue is that before PerIsolateMessageListener()
11-
// is invoked by v8, v8 preserves the vm state, which is JS. However,
12-
// SetIdle() requires the vm state is either EXTERNAL or IDLE when embedder
13-
// calling it.
12+
// is invoked by v8, v8 preserves the JS vm state, although it should
13+
// switch to EXTERNEL. https://bugs.chromium.org/p/v8/issues/detail?id=13464
14+
//
15+
// Therefore, this commit can be considered as an workaround of the v8 bug,
16+
// but we also find it not useful to call SetIdle() when terminating.
1417

1518
if (process.argv[2] === 'child') {
1619
const { Worker } = require('worker_threads');

0 commit comments

Comments
 (0)