Skip to content

Commit

Permalink
src: no call to SetIdle() when terminating
Browse files Browse the repository at this point in the history
Calling SetIdle() when terminating is not harmless.
When node terminates due to an unhandled exception,
v8 preseves the vm state, which is JS and notifies
node through PerIsolateMessageListener(). If node
calls SetIdle() later, v8 complains because it
requires the vm state to either be EXTERNEL or IDLE
when embedder calling SetIdle().
  • Loading branch information
ywave620 committed Nov 11, 2022
1 parent e1ac4e9 commit c0e3840
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/api/callback.cc
Expand Up @@ -97,10 +97,11 @@ void InternalCallbackScope::Close() {
if (closed_) return;
closed_ = true;

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

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

if (!env_->can_call_into_js()) return;
auto perform_stopping_check = [&]() {
if (env_->is_stopping()) {
MarkAsFailed();
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-unhandled-exception-with-worker-inuse.js
@@ -0,0 +1,27 @@
'use strict';
require('../common');

// 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 EXTERNEL 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 message
assert.doesNotMatch(stderr, /node::DumpBacktrace/);
}

0 comments on commit c0e3840

Please sign in to comment.