Skip to content

Commit

Permalink
src: fix cb scope bugs involved in termination
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ywave620 committed Feb 20, 2023
2 parents c203921 + 60016bf commit 2c47493
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,22 @@ 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()) {
if (!env_->can_call_into_js()) {
MarkAsFailed();
env_->async_hooks()->clear_async_id_stack();
}
};
perform_stopping_check();

if (!env_->can_call_into_js()) 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);
}
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-unhandled-exception-with-worker-inuse.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'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() will fail.
//
// The root cause of this issue is that before PerIsolateMessageListener()
// is invoked by v8, v8 preserves the vm state, which is JS. However,
// SetIdle() requires the vm state is either EXTERNAL or IDLE when embedder
// calling it.

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);
}

0 comments on commit 2c47493

Please sign in to comment.