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
1 parent 2c47493 commit 53f19ce
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 7 deletions.
4 changes: 2 additions & 2 deletions src/api/callback.cc
Expand Up @@ -101,14 +101,14 @@ void InternalCallbackScope::Close() {
// async id stack or pop the topmost one from it

auto perform_stopping_check = [&]() {
if (!env_->can_call_into_js()) {
if (env_->is_stopping()) {
MarkAsFailed();
env_->async_hooks()->clear_async_id_stack();
}
};
perform_stopping_check();

if (!env_->can_call_into_js()) return;
if (env_->is_stopping()) return;

Isolate* isolate = env_->isolate();
auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });
Expand Down
1 change: 1 addition & 0 deletions src/api/environment.cc
Expand Up @@ -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();
Expand Down
13 changes: 8 additions & 5 deletions test/parallel/test-unhandled-exception-with-worker-inuse.js
Expand Up @@ -3,14 +3,17 @@ const common = require('../common');

// https://github.com/nodejs/node/issues/45421
//
// Check that node will not call v8::Isolate::SetIdle() when exiting
// 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.
// 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 vm state, which is JS. However,
// SetIdle() requires the vm state is either EXTERNAL or IDLE when embedder
// calling it.
// 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');
Expand Down

0 comments on commit 53f19ce

Please sign in to comment.