From 663bb973ab105002c3fe049f6ecf91079e014ec0 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 13 May 2024 14:08:57 +0900 Subject: [PATCH] src: fix Worker termination in `inspector.waitForDebugger` Signed-off-by: Daeyeon Jeong PR-URL: https://github.com/nodejs/node/pull/52527 Fixes: https://github.com/nodejs/node/issues/52467 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Rich Trott Reviewed-By: Kohei Ueno --- src/env.cc | 7 +++ src/inspector/main_thread_interface.cc | 11 ++++- src/inspector/main_thread_interface.h | 5 ++ src/inspector_agent.cc | 21 +++++++++ src/inspector_agent.h | 2 + ...ctor-exit-worker-in-wait-for-connection.js | 46 +++++++++++++++++++ 6 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-inspector-exit-worker-in-wait-for-connection.js diff --git a/src/env.cc b/src/env.cc index b8574fcfb775b8..3d36416cdfa0f9 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1110,6 +1110,13 @@ void Environment::InitializeCompileCache() { void Environment::ExitEnv(StopFlags::Flags flags) { // Should not access non-thread-safe methods here. set_stopping(true); + +#if HAVE_INSPECTOR + if (inspector_agent_) { + inspector_agent_->StopIfWaitingForConnect(); + } +#endif + if ((flags & StopFlags::kDoNotTerminateIsolate) == 0) isolate_->TerminateExecution(); SetImmediateThreadsafe([](Environment* env) { diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index 0a0afe8aeda0a9..0fca7483e95c65 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -225,11 +225,20 @@ bool MainThreadInterface::WaitForFrontendEvent() { dispatching_messages_ = false; if (dispatching_message_queue_.empty()) { Mutex::ScopedLock scoped_lock(requests_lock_); - while (requests_.empty()) incoming_message_cond_.Wait(scoped_lock); + while (!stop_waiting_for_frontend_event_requested_ && requests_.empty()) { + incoming_message_cond_.Wait(scoped_lock); + } + stop_waiting_for_frontend_event_requested_ = false; } return true; } +void MainThreadInterface::StopWaitingForFrontendEvent() { + Mutex::ScopedLock scoped_lock(requests_lock_); + stop_waiting_for_frontend_event_requested_ = true; + incoming_message_cond_.Broadcast(scoped_lock); +} + void MainThreadInterface::DispatchMessages() { if (dispatching_messages_) return; diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index 71989db3bcacb9..35997adbe5ac66 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -78,6 +78,7 @@ class MainThreadInterface : void DispatchMessages(); void Post(std::unique_ptr request); bool WaitForFrontendEvent(); + void StopWaitingForFrontendEvent(); std::shared_ptr GetHandle(); Agent* inspector_agent() { return agent_; @@ -94,6 +95,10 @@ class MainThreadInterface : // when we reenter the DispatchMessages function. MessageQueue dispatching_message_queue_; bool dispatching_messages_ = false; + // This flag indicates an internal request to exit the loop in + // WaitForFrontendEvent(). It's set to true by calling + // StopWaitingForFrontendEvent(). + bool stop_waiting_for_frontend_event_requested_ = false; ConditionVariable incoming_message_cond_; // Used from any thread Agent* const agent_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 0e7b8fd1c2b3e5..16021a9d19e8d5 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -477,6 +477,20 @@ class NodeInspectorClient : public V8InspectorClient { } } + void StopIfWaitingForFrontendEvent() { + if (!waiting_for_frontend_) { + return; + } + waiting_for_frontend_ = false; + for (const auto& id_channel : channels_) { + id_channel.second->unsetWaitingForDebugger(); + } + + if (interface_) { + interface_->StopWaitingForFrontendEvent(); + } + } + int connectFrontend(std::unique_ptr delegate, bool prevent_shutdown) { int session_id = next_session_id_++; @@ -1024,6 +1038,13 @@ void Agent::WaitForConnect() { client_->waitForFrontend(); } +void Agent::StopIfWaitingForConnect() { + if (client_ == nullptr) { + return; + } + client_->StopIfWaitingForFrontendEvent(); +} + std::shared_ptr Agent::GetWorkerManager() { THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_, permission::PermissionScope::kInspector, diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 01c5f70be6f860..0f27aff61a3955 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -61,6 +61,8 @@ class Agent { // Blocks till frontend connects and sends "runIfWaitingForDebugger" void WaitForConnect(); + void StopIfWaitingForConnect(); + // Blocks till all the sessions with "WaitForDisconnectOnShutdown" disconnect void WaitForDisconnect(); void ReportUncaughtException(v8::Local error, diff --git a/test/parallel/test-inspector-exit-worker-in-wait-for-connection.js b/test/parallel/test-inspector-exit-worker-in-wait-for-connection.js new file mode 100644 index 00000000000000..4fcbb092fd23cf --- /dev/null +++ b/test/parallel/test-inspector-exit-worker-in-wait-for-connection.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const { parentPort, workerData, Worker } = require('node:worker_threads'); +if (!workerData) { + common.skipIfWorker(); +} + +const inspector = require('node:inspector'); +const assert = require('node:assert'); + +let TIMEOUT = common.platformTimeout(5000); +if (common.isWindows) { + // Refs: https://github.com/nodejs/build/issues/3014 + TIMEOUT = common.platformTimeout(15000); +} + +// Refs: https://github.com/nodejs/node/issues/52467 + +(async () => { + if (!workerData) { + // worker.terminate() should terminate the worker and the pending + // inspector.waitForDebugger(). + { + const worker = new Worker(__filename, { workerData: {} }); + await new Promise((r) => worker.on('message', r)); + await new Promise((r) => setTimeout(r, TIMEOUT)); + worker.on('exit', common.mustCall()); + await worker.terminate(); + } + // process.exit() should kill the process. + { + const worker = new Worker(__filename, { workerData: {} }); + await new Promise((r) => worker.on('message', r)); + await new Promise((r) => setTimeout(r, TIMEOUT)); + process.on('exit', (status) => assert.strictEqual(status, 0)); + setImmediate(() => process.exit()); + } + } else { + inspector.open(0, undefined, false); + parentPort.postMessage('open'); + inspector.waitForDebugger(); + } +})().then(common.mustCall());