Skip to content

Commit 2c47493

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
2 parents c203921 + 60016bf commit 2c47493

File tree

3 files changed

+37
-4
lines changed

3 files changed

+37
-4
lines changed

src/api/callback.cc

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,18 +97,22 @@ void InternalCallbackScope::Close() {
9797
if (closed_) return;
9898
closed_ = true;
9999

100-
Isolate* isolate = env_->isolate();
101-
auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });
100+
// This function must ends up with either cleanup the
101+
// async id stack or pop the topmost one from it
102102

103-
if (!env_->can_call_into_js()) return;
104103
auto perform_stopping_check = [&]() {
105-
if (env_->is_stopping()) {
104+
if (!env_->can_call_into_js()) {
106105
MarkAsFailed();
107106
env_->async_hooks()->clear_async_id_stack();
108107
}
109108
};
110109
perform_stopping_check();
111110

111+
if (!env_->can_call_into_js()) return;
112+
113+
Isolate* isolate = env_->isolate();
114+
auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });
115+
112116
if (!failed_ && async_context_.async_id != 0 && !skip_hooks_) {
113117
AsyncWrap::EmitAfter(env_, async_context_.async_id);
114118
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
'use strict';
2+
const common = require('../common');
3+
4+
// https://github.com/nodejs/node/issues/45421
5+
//
6+
// Check that node will not call v8::Isolate::SetIdle() when exiting
7+
// due to an unhandled exception, otherwise the assertion(enabled in
8+
// debug build only) in the SetIdle() will fail.
9+
//
10+
// 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.
14+
15+
if (process.argv[2] === 'child') {
16+
const { Worker } = require('worker_threads');
17+
new Worker('', { eval: true });
18+
throw new Error('xxx');
19+
} else {
20+
const assert = require('assert');
21+
const { spawnSync } = require('child_process');
22+
const result = spawnSync(process.execPath, [__filename, 'child']);
23+
24+
const stderr = result.stderr.toString().trim();
25+
// Expect error message to be preserved
26+
assert.match(stderr, /xxx/);
27+
// Expect no crash
28+
assert(!common.nodeProcessAborted(result.status, result.signal), stderr);
29+
}

0 commit comments

Comments
 (0)