From c0e3840b618ca1abc7fe453ca342804286f6aa51 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Fri, 11 Nov 2022 10:41:13 +0800 Subject: [PATCH 1/5] src: no call to SetIdle() when terminating 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(). --- src/api/callback.cc | 3 ++- ...t-unhandled-exception-with-worker-inuse.js | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-unhandled-exception-with-worker-inuse.js diff --git a/src/api/callback.cc b/src/api/callback.cc index 1287eb466fddd3..6a62937246803a 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -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(); diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js new file mode 100644 index 00000000000000..724759e587b501 --- /dev/null +++ b/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/); +} From 82f26a27c3372cdf057cf5a9a8782a8c63ab6b90 Mon Sep 17 00:00:00 2001 From: ywave620 <60539365+ywave620@users.noreply.github.com> Date: Mon, 14 Nov 2022 09:44:29 +0800 Subject: [PATCH 2/5] src: no call to SetIdle() when terminating Fix typo Co-authored-by: Ben Noordhuis --- test/parallel/test-unhandled-exception-with-worker-inuse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js index 724759e587b501..8007f3bd1accde 100644 --- a/test/parallel/test-unhandled-exception-with-worker-inuse.js +++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js @@ -7,7 +7,7 @@ require('../common'); // // 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 +// SetIdle() requires the vm state is either EXTERNAL or IDLE when embedder // calling it. if (process.argv[2] === 'child') { From 6bb4c8678c7e758245711b1c41a04be6959eb502 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Wed, 16 Nov 2022 09:57:20 +0800 Subject: [PATCH 3/5] src: no call to SetIdle() when terminating use fixture function to check no abort --- .../test-unhandled-exception-with-worker-inuse.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js index 8007f3bd1accde..e0a4d30db8deb3 100644 --- a/test/parallel/test-unhandled-exception-with-worker-inuse.js +++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js @@ -1,6 +1,8 @@ 'use strict'; -require('../common'); +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. @@ -22,6 +24,6 @@ if (process.argv[2] === '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/); + // Expect no crash + common.nodeProcessAborted(result.status, result.signal); } From 5a71cc0900890058b955f74995d88be599e9c765 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Wed, 16 Nov 2022 12:13:21 +0800 Subject: [PATCH 4/5] src: no call to SetIdle() when terminating add assertion --- test/parallel/test-unhandled-exception-with-worker-inuse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js index e0a4d30db8deb3..86de99c35c1566 100644 --- a/test/parallel/test-unhandled-exception-with-worker-inuse.js +++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js @@ -25,5 +25,5 @@ if (process.argv[2] === 'child') { // Expect error message to be preserved assert.match(stderr, /xxx/); // Expect no crash - common.nodeProcessAborted(result.status, result.signal); + assert.ok(common.nodeProcessAborted(result.status, result.signal), stderr); } From 24e94486b6f06069f7029ea11d0b3d166f986702 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Wed, 16 Nov 2022 12:15:34 +0800 Subject: [PATCH 5/5] src: no call to SetIdle() when terminating fix judgment bug --- test/parallel/test-unhandled-exception-with-worker-inuse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js index 86de99c35c1566..fd6d60da7f7a9e 100644 --- a/test/parallel/test-unhandled-exception-with-worker-inuse.js +++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js @@ -25,5 +25,5 @@ if (process.argv[2] === 'child') { // Expect error message to be preserved assert.match(stderr, /xxx/); // Expect no crash - assert.ok(common.nodeProcessAborted(result.status, result.signal), stderr); + assert.ok(!common.nodeProcessAborted(result.status, result.signal), stderr); }