From d659ed6dbec914301555feeacc2a009e1889d0d1 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sun, 2 Jun 2019 15:09:57 +0200
Subject: [PATCH] worker: refactor `worker.terminate()`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

At the collaborator summit in Berlin, the behaviour of
`worker.terminate()` was discussed.

In particular, switching from a callback-based to a Promise-based API
was suggested. While investigating that possibility later, it was
discovered that `.terminate()` was unintentionally synchronous up
until now (including calling its callback synchronously).

Also, the topic of its stability has been brought up. I have performed
two manual reviews of the native codebase for compatibility with
`.terminate()`, and performed some manual fuzz testing with the test
suite. At this point, bugs with `.terminate()` should, in my opinion,
be treated like bugs in other Node.js features.
(It is possible to make Node.js crash with `.terminate()` by messing
with internals and/or built-in prototype objects, but that is already
the case without `.terminate()` as well.)

This commit:

- Makes `.terminate()` an asynchronous operation.
- Makes `.terminate()` return a `Promise`.
- Runtime-deprecates passing a callback.
- Removes a warning about its stability from the documentation.
- Eliminates an unnecessary extra function from the C++ code.

A possible alternative to returning a `Promise` would be to keep the
method synchronous and just drop the callback. Generally, providing
an asynchronous API does provide us with a bit more flexibility.

Refs: https://github.com/nodejs/summit/issues/141

PR-URL: https://github.com/nodejs/node/pull/28021
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/deprecations.md                       | 15 +++++++++++
 doc/api/worker_threads.md                     | 26 +++++++++----------
 lib/internal/worker.js                        | 14 +++++++++-
 src/node_worker.cc                            |  7 +----
 src/node_worker.h                             |  1 -
 test/parallel/test-worker-dns-terminate.js    |  2 +-
 .../test-worker-nexttick-terminate.js         |  8 +++++-
 7 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index e375e7a5ba12be..c5588bc257564a 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2477,6 +2477,20 @@ The legacy HTTP parser, used by default in versions of Node.js prior to 12.0.0,
 is deprecated. This deprecation applies to users of the
 [`--http-parser=legacy`][] command-line flag.
 
+<a id="DEP0XXX"></a>
+### DEP0XXX: worker.terminate() with callback
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/28021
+    description: Runtime deprecation.
+-->
+
+Type: Runtime
+
+Passing a callback to [`worker.terminate()`][] is deprecated. Use the returned
+`Promise` instead, or a listener to the worker’s `'exit'` event.
+
 [`--http-parser=legacy`]: cli.html#cli_http_parser_library
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
@@ -2569,6 +2583,7 @@ is deprecated. This deprecation applies to users of the
 [`util.types`]: util.html#util_util_types
 [`util`]: util.html
 [`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect
+[`worker.terminate()`]: worker_threads.html#worker_threads_worker_terminate
 [`zlib.bytesWritten`]: zlib.html#zlib_zlib_byteswritten
 [Legacy URL API]: url.html#url_legacy_url_api
 [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 27d4fb2a6a62fb..39d948ccea7108 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -617,24 +617,23 @@ inside the worker thread. If `stdout: true` was not passed to the
 [`Worker`][] constructor, then data will be piped to the parent thread's
 [`process.stdout`][] stream.
 
-### worker.terminate([callback])
+### worker.terminate()
 <!-- YAML
 added: v10.5.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/28021
+    description: This function now returns a Promise.
+                 Passing a callback is deprecated, and was useless up to this
+                 version, as the Worker was actually terminated synchronously.
+                 Terminating is now a fully asynchronous operation.
 -->
 
-* `callback` {Function}
-  * `err` {Error}
-  * `exitCode` {integer}
+* Returns: {Promise}
 
 Stop all JavaScript execution in the worker thread as soon as possible.
-`callback` is an optional function that is invoked once this operation is known
-to have completed.
-
-**Warning**: Currently, not all code in the internals of Node.js is prepared to
-expect termination at arbitrary points in time and may crash if it encounters
-that condition. Consequently, only call `.terminate()` if it is known that the
-Worker thread is not accessing Node.js core modules other than what is exposed
-in the `worker` module.
+Returns a Promise for the exit code that is fulfilled when the
+[`'exit'` event][] is emitted.
 
 ### worker.threadId
 <!-- YAML
@@ -657,6 +656,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 `unref()` again will have no effect.
 
 [`'close'` event]: #worker_threads_event_close
+[`'exit'` event]: #worker_threads_event_exit
 [`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource
 [`Buffer`]: buffer.html
 [`EventEmitter`]: events.html
@@ -690,7 +690,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`worker.on('message')`]: #worker_threads_event_message_1
 [`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist
 [`worker.SHARE_ENV`]: #worker_threads_worker_share_env
-[`worker.terminate()`]: #worker_threads_worker_terminate_callback
+[`worker.terminate()`]: #worker_threads_worker_terminate
 [`worker.threadId`]: #worker_threads_worker_threadid_1
 [Addons worker support]: addons.html#addons_worker_support
 [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index fc2a5eceeddf8e..8fbff4fcf2cacf 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -228,10 +228,22 @@ class Worker extends EventEmitter {
 
     debug(`[${threadId}] terminates Worker with ID ${this.threadId}`);
 
-    if (typeof callback !== 'undefined')
+    if (typeof callback === 'function') {
+      process.emitWarning(
+        'Passing a callback to worker.terminate() is deprecated. ' +
+        'It returns a Promise instead.',
+        'DeprecationWarning', 'DEP0XXX');
       this.once('exit', (exitCode) => callback(null, exitCode));
+    }
 
     this[kHandle].stopThread();
+
+    // Do not use events.once() here, because the 'exit' event will always be
+    // emitted regardless of any errors, and the point is to only resolve
+    // once the thread has actually stopped.
+    return new Promise((resolve) => {
+      this.once('exit', resolve);
+    });
   }
 
   ref() {
diff --git a/src/node_worker.cc b/src/node_worker.cc
index c05e3a9c14c733..8f97f5c3514639 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -352,11 +352,8 @@ void Worker::JoinThread() {
   thread_joined_ = true;
 
   env()->remove_sub_worker_context(this);
-  OnThreadStopped();
   on_thread_finished_.Uninstall();
-}
 
-void Worker::OnThreadStopped() {
   {
     HandleScope handle_scope(env()->isolate());
     Context::Scope context_scope(env()->context());
@@ -370,7 +367,7 @@ void Worker::OnThreadStopped() {
     MakeCallback(env()->onexit_string(), 1, &code);
   }
 
-  // JoinThread() cleared all libuv handles bound to this Worker,
+  // We cleared all libuv handles bound to this Worker above,
   // the C++ object is no longer needed for anything now.
   MakeWeak();
 }
@@ -534,8 +531,6 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
 
   Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_);
   w->Exit(1);
-  w->JoinThread();
-  delete w;
 }
 
 void Worker::Ref(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_worker.h b/src/node_worker.h
index 0e659a9f522cda..db3c95d2aef965 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -52,7 +52,6 @@ class Worker : public AsyncWrap {
   static void Unref(const v8::FunctionCallbackInfo<v8::Value>& args);
 
  private:
-  void OnThreadStopped();
   void CreateEnvMessagePort(Environment* env);
 
   std::shared_ptr<PerIsolateOptions> per_isolate_opts_;
diff --git a/test/parallel/test-worker-dns-terminate.js b/test/parallel/test-worker-dns-terminate.js
index 381c211a88fb29..da9d543c3b0be2 100644
--- a/test/parallel/test-worker-dns-terminate.js
+++ b/test/parallel/test-worker-dns-terminate.js
@@ -10,5 +10,5 @@ require('worker_threads').parentPort.postMessage('0');
 
 w.on('message', common.mustCall(() => {
   // This should not crash the worker during a DNS request.
-  w.terminate(common.mustCall());
+  w.terminate().then(common.mustCall());
 }));
diff --git a/test/parallel/test-worker-nexttick-terminate.js b/test/parallel/test-worker-nexttick-terminate.js
index 269509178fac6e..9a22605bb54491 100644
--- a/test/parallel/test-worker-nexttick-terminate.js
+++ b/test/parallel/test-worker-nexttick-terminate.js
@@ -12,8 +12,14 @@ process.nextTick(() => {
 });
 `, { eval: true });
 
+// Test deprecation of .terminate() with callback.
+common.expectWarning(
+  'DeprecationWarning',
+  'Passing a callback to worker.terminate() is deprecated. ' +
+  'It returns a Promise instead.', 'DEP0XXX');
+
 w.on('message', common.mustCall(() => {
   setTimeout(() => {
-    w.terminate(common.mustCall());
+    w.terminate(common.mustCall()).then(common.mustCall());
   }, 1);
 }));