src: skip inspector wait in internal workers

Internal workers are essential to load user scripts and bootstrapped with internal entrypoints. They should not be waiting for inspectors even when `--inspect-brk` and `--inspect-wait` were specified, and avoid blocking main thread to bootstrap. IsolateData can be created with a specified PerIsolateOptions instead of creating a copy from the per_process namespace. This also avoids creating a copy bypassing the parent env's modified options, like creating a worker thread from a worker thread. PR-URL: #54219 Fixes: #53681 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Aug 7, 2024 · 2bcf999 · 2bcf999
1 parent e9deab9
commit 2bcf999
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 28 deletions.
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -371,9 +371,8 @@ IsolateData* CreateIsolateData(
     MultiIsolatePlatform* platform,
     ArrayBufferAllocator* allocator,
     const EmbedderSnapshotData* embedder_snapshot_data) {
-  const SnapshotData* snapshot_data =
-      SnapshotData::FromEmbedderWrapper(embedder_snapshot_data);
-  return new IsolateData(isolate, loop, platform, allocator, snapshot_data);
+  return IsolateData::CreateIsolateData(
+      isolate, loop, platform, allocator, embedder_snapshot_data);
 }
 
 void FreeIsolateData(IsolateData* isolate_data) {

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -590,11 +590,6 @@ inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
   return options_;
 }
 
-inline void IsolateData::set_options(
-    std::shared_ptr<PerIsolateOptions> options) {
-  options_ = std::move(options);
-}
-
 template <typename Fn>
 void Environment::SetImmediate(Fn&& cb, CallbackFlags::Flags flags) {
   auto callback = native_immediates_.CreateCallback(std::move(cb), flags);

diff --git a/src/env.cc b/src/env.cc
@@ -538,19 +538,35 @@ Mutex IsolateData::isolate_data_mutex_;
 std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
     IsolateData::wrapper_data_map_;
 
+IsolateData* IsolateData::CreateIsolateData(
+    Isolate* isolate,
+    uv_loop_t* loop,
+    MultiIsolatePlatform* platform,
+    ArrayBufferAllocator* allocator,
+    const EmbedderSnapshotData* embedder_snapshot_data,
+    std::shared_ptr<PerIsolateOptions> options) {
+  const SnapshotData* snapshot_data =
+      SnapshotData::FromEmbedderWrapper(embedder_snapshot_data);
+  if (options == nullptr) {
+    options = per_process::cli_options->per_isolate->Clone();
+  }
+  return new IsolateData(
+      isolate, loop, platform, allocator, snapshot_data, options);
+}
+
 IsolateData::IsolateData(Isolate* isolate,
                          uv_loop_t* event_loop,
                          MultiIsolatePlatform* platform,
                          ArrayBufferAllocator* node_allocator,
-                         const SnapshotData* snapshot_data)
+                         const SnapshotData* snapshot_data,
+                         std::shared_ptr<PerIsolateOptions> options)
     : isolate_(isolate),
       event_loop_(event_loop),
       node_allocator_(node_allocator == nullptr ? nullptr
                                                 : node_allocator->GetImpl()),
       platform_(platform),
-      snapshot_data_(snapshot_data) {
-  options_.reset(
-      new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
+      snapshot_data_(snapshot_data),
+      options_(std::move(options)) {
   v8::CppHeap* cpp_heap = isolate->GetCppHeap();
 
   uint16_t cppgc_id = kDefaultCppGCEmbedderID;

diff --git a/src/env.h b/src/env.h
@@ -139,12 +139,22 @@ struct PerIsolateWrapperData {
 };
 
 class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
- public:
+ private:
   IsolateData(v8::Isolate* isolate,
               uv_loop_t* event_loop,
-              MultiIsolatePlatform* platform = nullptr,
-              ArrayBufferAllocator* node_allocator = nullptr,
-              const SnapshotData* snapshot_data = nullptr);
+              MultiIsolatePlatform* platform,
+              ArrayBufferAllocator* node_allocator,
+              const SnapshotData* snapshot_data,
+              std::shared_ptr<PerIsolateOptions> options);
+
+ public:
+  static IsolateData* CreateIsolateData(
+      v8::Isolate* isolate,
+      uv_loop_t* event_loop,
+      MultiIsolatePlatform* platform = nullptr,
+      ArrayBufferAllocator* node_allocator = nullptr,
+      const EmbedderSnapshotData* embedder_snapshot_data = nullptr,
+      std::shared_ptr<PerIsolateOptions> options = nullptr);
   ~IsolateData();
 
   SET_MEMORY_INFO_NAME(IsolateData)
@@ -173,7 +183,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
   inline MultiIsolatePlatform* platform() const;
   inline const SnapshotData* snapshot_data() const;
   inline std::shared_ptr<PerIsolateOptions> options();
-  inline void set_options(std::shared_ptr<PerIsolateOptions> options);
 
   inline NodeArrayBufferAllocator* node_allocator() const;
 

diff --git a/src/node_options-inl.h b/src/node_options-inl.h
@@ -17,6 +17,13 @@ EnvironmentOptions* PerIsolateOptions::get_per_env_options() {
   return per_env.get();
 }
 
+std::shared_ptr<PerIsolateOptions> PerIsolateOptions::Clone() const {
+  auto options =
+      std::shared_ptr<PerIsolateOptions>(new PerIsolateOptions(*this));
+  options->per_env = std::make_shared<EnvironmentOptions>(*per_env);
+  return options;
+}
+
 namespace options_parser {
 
 template <typename Options>

diff --git a/src/node_options.h b/src/node_options.h
@@ -94,6 +94,11 @@ class DebugOptions : public Options {
     break_first_line = true;
   }
 
+  void DisableWaitOrBreakFirstLine() {
+    inspect_wait = false;
+    break_first_line = false;
+  }
+
   bool wait_for_connect() const {
     return break_first_line || break_node_first_line || inspect_wait;
   }
@@ -251,6 +256,9 @@ class EnvironmentOptions : public Options {
 
 class PerIsolateOptions : public Options {
  public:
+  PerIsolateOptions() = default;
+  PerIsolateOptions(PerIsolateOptions&&) = default;
+
   std::shared_ptr<EnvironmentOptions> per_env { new EnvironmentOptions() };
   bool track_heap_objects = false;
   bool report_uncaught_exception = false;
@@ -262,6 +270,11 @@ class PerIsolateOptions : public Options {
   inline EnvironmentOptions* get_per_env_options();
   void CheckOptions(std::vector<std::string>* errors,
                     std::vector<std::string>* argv) override;
+
+  inline std::shared_ptr<PerIsolateOptions> Clone() const;
+
+ private:
+  PerIsolateOptions(const PerIsolateOptions&) = default;
 };
 
 class PerProcessOptions : public Options {

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -185,16 +185,15 @@ class WorkerThreadData {
       isolate->SetStackLimit(w->stack_base_);
 
       HandleScope handle_scope(isolate);
-      isolate_data_.reset(
-          CreateIsolateData(isolate,
-                            &loop_,
-                            w_->platform_,
-                            allocator.get(),
-                            w->snapshot_data()->AsEmbedderWrapper().get()));
+      isolate_data_.reset(IsolateData::CreateIsolateData(
+          isolate,
+          &loop_,
+          w_->platform_,
+          allocator.get(),
+          w->snapshot_data()->AsEmbedderWrapper().get(),
+          std::move(w_->per_isolate_opts_)));
       CHECK(isolate_data_);
       CHECK(!isolate_data_->is_building_snapshot());
-      if (w_->per_isolate_opts_)
-        isolate_data_->set_options(std::move(w_->per_isolate_opts_));
       isolate_data_->set_worker_context(w_);
       isolate_data_->max_young_gen_size =
           params.constraints.max_young_generation_size_in_bytes();
@@ -491,9 +490,8 @@ Worker::~Worker() {
 
 void Worker::New(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  auto is_internal = args[5];
-  CHECK(is_internal->IsBoolean());
-  if (is_internal->IsFalse()) {
+  bool is_internal = args[5]->IsTrue();
+  if (!is_internal) {
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, permission::PermissionScope::kWorkerThreads, "");
   }
@@ -658,7 +656,19 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
       return;
     }
   } else {
+    // Copy the parent's execArgv.
     exec_argv_out = env->exec_argv();
+    per_isolate_opts = env->isolate_data()->options()->Clone();
+  }
+
+  // Internal workers should not wait for inspector frontend to connect or
+  // break on the first line of internal scripts. Module loader threads are
+  // essential to load user codes and must not be blocked by the inspector
+  // for internal scripts.
+  // Still, `--inspect-node` can break on the first line of internal scripts.
+  if (is_internal) {
+    per_isolate_opts->per_env->get_debug_options()
+        ->DisableWaitOrBreakFirstLine();
   }
 
   const SnapshotData* snapshot_data = env->isolate_data()->snapshot_data();

diff --git a/test/common/inspector-helper.js b/test/common/inspector-helper.js
@@ -309,6 +309,10 @@ class InspectorSession {
       return notification.method === 'Runtime.executionContextDestroyed' &&
         notification.params.executionContextId === 1;
     });
+    await this.waitForDisconnect();
+  }
+
+  async waitForDisconnect() {
     while ((await this._instance.nextStderrString()) !==
               'Waiting for the debugger to disconnect...');
     await this.disconnect();

diff --git a/test/parallel/test-esm-loader-hooks-inspect-brk.js b/test/parallel/test-esm-loader-hooks-inspect-brk.js
@@ -0,0 +1,34 @@
+// This tests esm loader's internal worker will not be blocked by --inspect-brk.
+// Regression: https://github.com/nodejs/node/issues/53681
+
+'use strict';
+const common = require('../common');
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const { NodeInstance } = require('../common/inspector-helper.js');
+
+async function runIfWaitingForDebugger(session) {
+  const commands = [
+    { 'method': 'Runtime.enable' },
+    { 'method': 'Debugger.enable' },
+    { 'method': 'Runtime.runIfWaitingForDebugger' },
+  ];
+
+  await session.send(commands);
+  await session.waitForNotification('Debugger.paused');
+}
+
+async function runTest() {
+  const main = fixtures.path('es-module-loaders', 'register-loader.mjs');
+  const child = new NodeInstance(['--inspect-brk=0'], '', main);
+
+  const session = await child.connectInspectorSession();
+  await runIfWaitingForDebugger(session);
+  await session.runToCompletion();
+  assert.strictEqual((await child.expectShutdown()).exitCode, 0);
+}
+
+runTest();
diff --git a/test/parallel/test-esm-loader-hooks-inspect-wait.js b/test/parallel/test-esm-loader-hooks-inspect-wait.js
@@ -0,0 +1,33 @@
+// This tests esm loader's internal worker will not be blocked by --inspect-wait.
+// Regression: https://github.com/nodejs/node/issues/53681
+
+'use strict';
+const common = require('../common');
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const { NodeInstance } = require('../common/inspector-helper.js');
+
+async function runIfWaitingForDebugger(session) {
+  const commands = [
+    { 'method': 'Runtime.enable' },
+    { 'method': 'Debugger.enable' },
+    { 'method': 'Runtime.runIfWaitingForDebugger' },
+  ];
+
+  await session.send(commands);
+}
+
+async function runTest() {
+  const main = fixtures.path('es-module-loaders', 'register-loader.mjs');
+  const child = new NodeInstance(['--inspect-wait=0'], '', main);
+
+  const session = await child.connectInspectorSession();
+  await runIfWaitingForDebugger(session);
+  await session.waitForDisconnect();
+  assert.strictEqual((await child.expectShutdown()).exitCode, 0);
+}
+
+runTest();