From d55fac1fff942695dc34d18b999312d7881d1a8e Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 22 Jan 2020 23:23:21 +0100
Subject: [PATCH 1/2] 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
---
 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 785f2783c22346..078b6ac5bbf648 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -438,8 +438,6 @@ void Worker::JoinThread() {
 }
 
 Worker::~Worker() {
-  JoinThread();
-
   Mutex::ScopedLock lock(mutex_);
 
   CHECK(stopped_);
@@ -599,6 +597,7 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
         [w = std::unique_ptr<Worker>(w)](Environment* env) {
           if (w->has_ref_)
             env->add_refs(-1);
+          w->JoinThread();
           // implicitly delete w
         });
   }, static_cast<void*>(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'); }));

From ff3cd579777d6c9a5dfcacd153c4ba05924750db Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 22 Jan 2020 23:33:29 +0100
Subject: [PATCH 2/2] src: harden running native `SetImmediate()`s slightly

Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.
---
 src/env.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/env.cc b/src/env.cc
index ba5e9cd5a05218..19c29c2fcab950 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -698,6 +698,8 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
       if (head->is_refed() || !only_refed)
         head->Call(this);
 
+      head.reset();  // Destroy now so that this is also observed by try_catch.
+
       if (UNLIKELY(try_catch.HasCaught())) {
         if (!try_catch.HasTerminated() && can_call_into_js())
           errors::TriggerUncaughtException(isolate(), try_catch);