From 3cc74d0e3a477d7f2ff32ca0dcdaa055097f8be9 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 5 Aug 2024 22:47:29 +0100 Subject: [PATCH] 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. --- src/api/environment.cc | 5 ++- src/env-inl.h | 5 --- src/env.cc | 24 ++++++++++--- src/env.h | 19 ++++++++--- src/node_options-inl.h | 7 ++++ src/node_options.h | 13 +++++++ src/node_worker.cc | 32 +++++++++++------ .../test-esm-loader-hooks-inspect-brk.js | 34 +++++++++++++++++++ 8 files changed, 111 insertions(+), 28 deletions(-) create mode 100644 test/parallel/test-esm-loader-hooks-inspect-brk.js diff --git a/src/api/environment.cc b/src/api/environment.cc index 6524eb440bd69f..77c20a4b6b9db4 100644 --- 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 index 27f71072e26100..fcc8cfdd92429f 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -590,11 +590,6 @@ inline std::shared_ptr IsolateData::options() { return options_; } -inline void IsolateData::set_options( - std::shared_ptr options) { - options_ = std::move(options); -} - template 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 index 799b36aaebdded..096f0752e23f29 100644 --- a/src/env.cc +++ b/src/env.cc @@ -538,19 +538,35 @@ Mutex IsolateData::isolate_data_mutex_; std::unordered_map> 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 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 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 index 5bc5bf061b424f..85bce29fcba427 100644 --- 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 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 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 options(); - inline void set_options(std::shared_ptr options); inline NodeArrayBufferAllocator* node_allocator() const; diff --git a/src/node_options-inl.h b/src/node_options-inl.h index 4b4b9f9b4af7b2..fabf3be149be93 100644 --- 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::Clone() const { + auto options = + std::shared_ptr(new PerIsolateOptions(*this)); + options->per_env = std::make_shared(*per_env); + return options; +} + namespace options_parser { template diff --git a/src/node_options.h b/src/node_options.h index cbc1b58af9ba64..a2ed8b1cd4d1d0 100644 --- 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 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* errors, std::vector* argv) override; + + inline std::shared_ptr Clone() const; + + private: + PerIsolateOptions(const PerIsolateOptions&) = default; }; class PerProcessOptions : public Options { diff --git a/src/node_worker.cc b/src/node_worker.cc index 571baf88b94691..a946939a88fc58 100644 --- 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& 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& 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/parallel/test-esm-loader-hooks-inspect-brk.js b/test/parallel/test-esm-loader-hooks-inspect-brk.js new file mode 100644 index 00000000000000..881bdfd2dd101a --- /dev/null +++ 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();