From c5de212039bd6dc40acf3a9aff5c58d5295c4bd1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 22 Jan 2020 23:23:21 +0100 Subject: [PATCH] worker: move JoinThread() back into exit callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit de2c68c7dd17a217a818ea881e433034006fdb4b moved this call to the destructor, under the assumption that that would essentially be equivalent to running it as part of the callback since the worker would be destroyed along with the callback. However, the actual code in `Environment::RunAndClearNativeImmediates()` comes with the subtlety that testing whether a JS exception has been thrown happens between the invocation of the callback and its destruction, leaving a possible exception from `JoinThread()` potentially unhandled (and unintentionally silenced through the `TryCatch`). This affected exceptions thrown from the `'exit'` event of the Worker, and made the `parallel/test-worker-message-type-unknown` test flaky, as the invalid message was sometimes only received during the Worker thread’s exit handler. Fix this by moving the `JoinThread()` call back to where it was before. Refs: https://github.com/nodejs/node/pull/31386 Backport-PR-URL: https://github.com/nodejs/node/pull/32301 PR-URL: https://github.com/nodejs/node/pull/31468 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil --- src/node_worker.cc | 3 +-- test/parallel/test-worker-exit-event-error.js | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-worker-exit-event-error.js diff --git a/src/node_worker.cc b/src/node_worker.cc index eeed824f66dee9..83597c1616a6d4 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -454,8 +454,6 @@ void Worker::JoinThread() { } Worker::~Worker() { - JoinThread(); - Mutex::ScopedLock lock(mutex_); CHECK(stopped_); @@ -655,6 +653,7 @@ void Worker::StartThread(const FunctionCallbackInfo& args) { [w = std::unique_ptr(w)](Environment* env) { if (w->has_ref_) env->add_refs(-1); + w->JoinThread(); // implicitly delete w }); }, static_cast(w)), 0); diff --git a/test/parallel/test-worker-exit-event-error.js b/test/parallel/test-worker-exit-event-error.js new file mode 100644 index 00000000000000..e2427c7dff726b --- /dev/null +++ b/test/parallel/test-worker-exit-event-error.js @@ -0,0 +1,8 @@ +'use strict'; +const common = require('../common'); +const { Worker } = require('worker_threads'); + +process.on('uncaughtException', common.mustCall()); + +new Worker('', { eval: true }) + .on('exit', common.mustCall(() => { throw new Error('foo'); }));