src: fix Worker termination when '--inspect-brk' is passed

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #53724
Fixes: #53648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Kohei Ueno <[email protected]>

Author: daeyeon

src/debug_utils.h

@@ -45,6 +45,7 @@ void NODE_EXTERN_PRIVATE FWrite(FILE* file, const std::string& str);
   V(DIAGNOSTICS)                                                               \
   V(HUGEPAGES)                                                                 \
   V(INSPECTOR_SERVER)                                                          \
+  V(INSPECTOR_CLIENT)                                                          \
   V(INSPECTOR_PROFILER)                                                        \
   V(CODE_CACHE)                                                                \
   V(NGTCP2_DEBUG)                                                              \

src/env-inl.h

@@ -664,6 +664,10 @@ inline bool Environment::should_create_inspector() const {
          !options_->test_runner && !options_->watch_mode;
 }
 
+inline bool Environment::should_wait_for_inspector_frontend() const {
+  return (flags_ & EnvironmentFlags::kNoWaitForInspectorFrontend) == 0;
+}
+
 inline bool Environment::tracks_unmanaged_fds() const {
   return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
 }

src/env.h

@@ -628,6 +628,7 @@ class Environment : public MemoryRetainer {
   // the ownership if transferred into the Environment.
   void InitializeInspector(
       std::unique_ptr<inspector::ParentInspectorHandle> parent_handle);
+  void WaitForInspectorFrontendByOptions();
 #endif
 
   inline size_t async_callback_scope_depth() const;
@@ -798,6 +799,7 @@ class Environment : public MemoryRetainer {
   inline bool no_native_addons() const;
   inline bool should_not_register_esm_loader() const;
   inline bool should_create_inspector() const;
+  inline bool should_wait_for_inspector_frontend() const;
   inline bool owns_process_state() const;
   inline bool owns_inspector() const;
   inline bool tracks_unmanaged_fds() const;

src/inspector_agent.cc

@@ -487,6 +487,8 @@ class NodeInspectorClient : public V8InspectorClient {
     }
 
     if (interface_) {
+      per_process::Debug(DebugCategory::INSPECTOR_CLIENT,
+                         "Stopping waiting for frontend events\n");
       interface_->StopWaitingForFrontendEvent();
     }
   }
@@ -668,11 +670,16 @@ class NodeInspectorClient : public V8InspectorClient {
 
     running_nested_loop_ = true;
 
+    per_process::Debug(DebugCategory::INSPECTOR_CLIENT,
+                       "Entering nested loop\n");
+
     while (shouldRunMessageLoop()) {
       if (interface_) interface_->WaitForFrontendEvent();
       env_->RunAndClearInterrupts();
     }
     running_nested_loop_ = false;
+
+    per_process::Debug(DebugCategory::INSPECTOR_CLIENT, "Exited nested loop\n");
   }
 
   double currentTimeMS() override {
@@ -759,27 +766,11 @@ bool Agent::Start(const std::string& path,
     }
   }, parent_env_);
 
-  bool wait_for_connect = options.wait_for_connect();
-  bool should_break_first_line = options.should_break_first_line();
-  if (parent_handle_) {
-    should_break_first_line = parent_handle_->WaitForConnect();
-    parent_handle_->WorkerStarted(client_->getThreadHandle(),
-                                  should_break_first_line);
-  } else if (!options.inspector_enabled || !options.allow_attaching_debugger ||
-             !StartIoThread()) {
+  if (!parent_handle_ &&
+      (!options.inspector_enabled || !options.allow_attaching_debugger ||
+       !StartIoThread())) {
     return false;
   }
-
-  if (wait_for_connect || should_break_first_line) {
-    // Patch the debug options to implement waitForDebuggerOnStart for
-    // the NodeWorker.enable method.
-    if (should_break_first_line) {
-      CHECK(!parent_env_->has_serialized_options());
-      debug_options_.EnableBreakFirstLine();
-      parent_env_->options()->get_debug_options()->EnableBreakFirstLine();
-    }
-    client_->waitForFrontend();
-  }
   return true;
 }
 
@@ -1038,6 +1029,33 @@ void Agent::WaitForConnect() {
   client_->waitForFrontend();
 }
 
+bool Agent::WaitForConnectByOptions() {
+  if (client_ == nullptr) {
+    return false;
+  }
+
+  bool wait_for_connect = debug_options_.wait_for_connect();
+  bool should_break_first_line = debug_options_.should_break_first_line();
+  if (parent_handle_) {
+    should_break_first_line = parent_handle_->WaitForConnect();
+    parent_handle_->WorkerStarted(client_->getThreadHandle(),
+                                  should_break_first_line);
+  }
+
+  if (wait_for_connect || should_break_first_line) {
+    // Patch the debug options to implement waitForDebuggerOnStart for
+    // the NodeWorker.enable method.
+    if (should_break_first_line) {
+      CHECK(!parent_env_->has_serialized_options());
+      debug_options_.EnableBreakFirstLine();
+      parent_env_->options()->get_debug_options()->EnableBreakFirstLine();
+    }
+    client_->waitForFrontend();
+    return true;
+  }
+  return false;
+}
+
 void Agent::StopIfWaitingForConnect() {
   if (client_ == nullptr) {
     return;

src/inspector_agent.h

@@ -61,6 +61,7 @@ class Agent {
 
   // Blocks till frontend connects and sends "runIfWaitingForDebugger"
   void WaitForConnect();
+  bool WaitForConnectByOptions();
   void StopIfWaitingForConnect();
 
   // Blocks till all the sessions with "WaitForDisconnectOnShutdown" disconnect

src/node.cc

@@ -204,7 +204,17 @@ void Environment::InitializeInspector(
     return;
   }
 
+  if (should_wait_for_inspector_frontend()) {
+    WaitForInspectorFrontendByOptions();
+  }
+
   profiler::StartProfilers(this);
+}
+
+void Environment::WaitForInspectorFrontendByOptions() {
+  if (!inspector_agent_->WaitForConnectByOptions()) {
+    return;
+  }
 
   if (inspector_agent_->options().break_node_first_line) {
     inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap");

src/node.h

@@ -661,7 +661,11 @@ enum Flags : uint64_t {
   // Controls where or not the InspectorAgent for this Environment should
   // call StartDebugSignalHandler.  This control is needed by embedders who may
   // not want to allow other processes to start the V8 inspector.
-  kNoStartDebugSignalHandler = 1 << 10
+  kNoStartDebugSignalHandler = 1 << 10,
+  // Controls whether the InspectorAgent created for this Environment waits for
+  // Inspector frontend events during the Environment creation. It's used to
+  // call node::Stop(env) on a Worker thread that is waiting for the events.
+  kNoWaitForInspectorFrontend = 1 << 11
 };
 }  // namespace EnvironmentFlags
 

src/node_worker.cc

@@ -359,6 +359,9 @@ void Worker::Run() {
       CHECK(!context.IsEmpty());
       Context::Scope context_scope(context);
       {
+#if HAVE_INSPECTOR
+        environment_flags_ |= EnvironmentFlags::kNoWaitForInspectorFrontend;
+#endif
         env_.reset(CreateEnvironment(
             data.isolate_data_.get(),
             context,
@@ -380,6 +383,10 @@ void Worker::Run() {
         this->env_ = env_.get();
       }
       Debug(this, "Created Environment for worker with id %llu", thread_id_.id);
+
+#if HAVE_INSPECTOR
+      this->env_->WaitForInspectorFrontendByOptions();
+#endif
       if (is_stopped()) return;
       {
         if (!CreateEnvMessagePort(env_.get())) {

test/parallel/test-inspector-exit-worker-in-wait-for-connection2.js

@@ -0,0 +1,47 @@
+'use strict';
+
+const common = require('../common');
+common.skipIfInspectorDisabled();
+
+const { workerData, Worker } = require('node:worker_threads');
+if (!workerData) {
+  common.skipIfWorker();
+}
+
+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/53648
+
+(async () => {
+  if (!workerData) {
+    // worker.terminate() should terminate the worker created with execArgv:
+    // ["--inspect-brk"].
+    {
+      const worker = new Worker(__filename, {
+        execArgv: ['--inspect-brk=0'],
+        workerData: {},
+      });
+      await new Promise((r) => setTimeout(r, TIMEOUT));
+      worker.on('exit', common.mustCall());
+      await worker.terminate();
+    }
+    // process.exit() should kill the process.
+    {
+      new Worker(__filename, {
+        execArgv: ['--inspect-brk=0'],
+        workerData: {},
+      });
+      await new Promise((r) => setTimeout(r, TIMEOUT));
+      process.on('exit', (status) => assert.strictEqual(status, 0));
+      setImmediate(() => process.exit());
+    }
+  } else {
+    console.log('Worker running!');
+  }
+})().then(common.mustCall());