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: 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>

2 files changed, 1 insertions, 7 deletions

diff --git a/src/node_worker.cc b/src/node_worker.cc
index c05e3a9c14..8f97f5c351 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 0e659a9f52..db3c95d2ae 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_;