From ed24c19002379063ce100037bfff7ca2da5b6cc8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 2 Jun 2019 15:09:57 +0200 Subject: 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 Reviewed-By: Rich Trott Reviewed-By: Ben Noordhuis Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Ruben Bridgewater Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell --- src/node_worker.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'src/node_worker.cc') 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& 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& args) { -- cgit v1.2.3