From 0830b83eda02f8b5f1013bba0b6b164e3aa7fcbe Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 2 Apr 2020 23:40:25 +0200 Subject: [PATCH 1/2] Revert "embedding: make Stop() stop Workers" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 037ac99ed5aa763b7a3567da2cc81f9d7b97bdf9. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: https://github.com/nodejs/node/pull/32531 --- src/api/environment.cc | 3 ++- src/env.cc | 3 +-- src/env.h | 2 +- src/node.cc | 2 +- src/node.h | 7 +++---- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 857506013fab8c..e5d4b27e67c8b6 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -725,7 +725,8 @@ ThreadId AllocateEnvironmentThreadId() { } void DefaultProcessExitHandler(Environment* env, int exit_code) { - Stop(env); + env->set_can_call_into_js(false); + env->stop_sub_worker_contexts(); DisposePlatform(); exit(exit_code); } diff --git a/src/env.cc b/src/env.cc index 0b2103e6e4aef8..3d8d69238ccbee 100644 --- a/src/env.cc +++ b/src/env.cc @@ -528,10 +528,9 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { } } -void Environment::Stop() { +void Environment::ExitEnv() { set_can_call_into_js(false); set_stopping(true); - stop_sub_worker_contexts(); isolate_->TerminateExecution(); SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); }); } diff --git a/src/env.h b/src/env.h index 61e222f02e9f2e..b3bc553cc75f1e 100644 --- a/src/env.h +++ b/src/env.h @@ -899,7 +899,7 @@ class Environment : public MemoryRetainer { void RegisterHandleCleanups(); void CleanupHandles(); void Exit(int code); - void Stop(); + void ExitEnv(); // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, diff --git a/src/node.cc b/src/node.cc index 302b277493c178..9e0e2464a02f4d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1059,7 +1059,7 @@ int Start(int argc, char** argv) { } int Stop(Environment* env) { - env->Stop(); + env->ExitEnv(); return 0; } diff --git a/src/node.h b/src/node.h index afa2859b5b9c7f..4278eedfcbe09e 100644 --- a/src/node.h +++ b/src/node.h @@ -224,8 +224,7 @@ class Environment; NODE_EXTERN int Start(int argc, char* argv[]); // Tear down Node.js while it is running (there are active handles -// in the loop and / or actively executing JavaScript code). This also stops -// all Workers that may have been started earlier. +// in the loop and / or actively executing JavaScript code). NODE_EXTERN int Stop(Environment* env); // TODO(addaleax): Officially deprecate this and replace it with something @@ -479,8 +478,8 @@ NODE_EXTERN void FreeEnvironment(Environment* env); // It receives the Environment* instance and the exit code as arguments. // This could e.g. call Stop(env); in order to terminate execution and stop // the event loop. -// The default handler calls Stop(), disposes of the global V8 platform -// instance, if one is being used, and calls exit(). +// The default handler disposes of the global V8 platform instance, if one is +// being used, and calls exit(). NODE_EXTERN void SetProcessExitHandler( Environment* env, std::function&& handler); From 80d4ba84506b9fdc07e84cb71e613652224d0307 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 2 Apr 2020 23:43:20 +0200 Subject: [PATCH 2/2] src,test: add regression test for nested Worker termination This adds a regression test for terminating a Worker inside which another Worker is running. --- src/env.cc | 2 ++ test/parallel/test-worker-terminate-nested.js | 15 +++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 test/parallel/test-worker-terminate-nested.js diff --git a/src/env.cc b/src/env.cc index 3d8d69238ccbee..bee70ae58e22f6 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1014,6 +1014,8 @@ void Environment::Exit(int exit_code) { } void Environment::stop_sub_worker_contexts() { + DCHECK_EQ(Isolate::GetCurrent(), isolate()); + while (!sub_worker_contexts_.empty()) { Worker* w = *sub_worker_contexts_.begin(); remove_sub_worker_context(w); diff --git a/test/parallel/test-worker-terminate-nested.js b/test/parallel/test-worker-terminate-nested.js new file mode 100644 index 00000000000000..3924528cea1ccb --- /dev/null +++ b/test/parallel/test-worker-terminate-nested.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../common'); +const { Worker } = require('worker_threads'); + +// Check that a Worker that's running another Worker can be terminated. + +const worker = new Worker(` +const { Worker, parentPort } = require('worker_threads'); +const worker = new Worker('setInterval(() => {}, 10);', { eval: true }); +worker.on('online', () => { + parentPort.postMessage({}); +}); +`, { eval: true }); + +worker.on('message', common.mustCall(() => worker.terminate()));