worker: refactor worker.terminate()

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: openjs-foundation/summit#141
nodejs · addaleax · Jun 2, 2019 · Jun 10, 2019 · 34efb187f6127688876e9d3d8e6af0363ca3a5b5
commit 34efb187f6127688876e9d3d8e6af0363ca3a5b5
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -2484,6 +2484,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
@@ -2576,6 +2590,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
@@ -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
@@ -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
@@ -350,11 +350,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());
@@ -368,7 +365,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();
 }
@@ -532,8 +529,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
@@ -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
@@ -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
@@ -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);
 }));