From 9083a676dd3e01e01cc551993f0b0d7711acbbf1 Mon Sep 17 00:00:00 2001
From: elyalvarado <elyalvarado@gmail.com>
Date: Fri, 21 Jun 2019 17:21:43 -0500
Subject: [PATCH] worker: handle calling terminate when kHandler is null

This PR makes a change to the Worker.terminate() when called if the
kHandler is null. Before this pull request it was returning undefined,
but the API is expecting a promise. With the changes in this PR if
terminate is called a Promise.resolve() is returned, unless a callback
is passed in which case the old behavior stays (returns undefined).

PR-URL: https://github.com/nodejs/node/pull/28370
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 lib/internal/worker.js                        |  5 ++--
 .../test-worker-terminate-null-handler.js     | 27 +++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-worker-terminate-null-handler.js

diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index b6a7bbad671..3cd5472b934 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -224,8 +224,6 @@ class Worker extends EventEmitter {
   }
 
   terminate(callback) {
-    if (this[kHandle] === null) return;
-
     debug(`[${threadId}] terminates Worker with ID ${this.threadId}`);
 
     if (typeof callback === 'function') {
@@ -233,9 +231,12 @@ class Worker extends EventEmitter {
         'Passing a callback to worker.terminate() is deprecated. ' +
         'It returns a Promise instead.',
         'DeprecationWarning', 'DEP0132');
+      if (this[kHandle] === null) return Promise.resolve();
       this.once('exit', (exitCode) => callback(null, exitCode));
     }
 
+    if (this[kHandle] === null) return Promise.resolve();
+
     this[kHandle].stopThread();
 
     // Do not use events.once() here, because the 'exit' event will always be
diff --git a/test/parallel/test-worker-terminate-null-handler.js b/test/parallel/test-worker-terminate-null-handler.js
new file mode 100644
index 00000000000..3807501de02
--- /dev/null
+++ b/test/parallel/test-worker-terminate-null-handler.js
@@ -0,0 +1,27 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+// Test that calling worker.terminate() if kHandler is null should return an
+// empty promise that resolves to undefined, even when a callback is passed
+
+const worker = new Worker(`
+const { parentPort } = require('worker_threads');
+parentPort.postMessage({ hello: 'world' });
+`, { eval: true });
+
+process.once('beforeExit', common.mustCall(() => {
+  console.log('beforeExit');
+  worker.ref();
+}));
+
+worker.on('exit', common.mustCall(() => {
+  console.log('exit');
+  worker.terminate().then((res) => assert.strictEqual(res, undefined));
+  worker.terminate(() => null).then(
+    (res) => assert.strictEqual(res, undefined)
+  );
+}));
+
+worker.unref();