FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: skip inspector wait in internal workers · nodejs/node@91936eb · GitHub

/ node Public

Commit 91936eb

Browse files
legendecas
authored and
targos
committed
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 <yagiz@nizipli.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
1 parent f90688c commit 91936eb

10 files changed

Lines changed: 148 additions & 28 deletions

‎src/api/environment.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -371,9 +371,8 @@ IsolateData* CreateIsolateData(
371371
MultiIsolatePlatform* platform,
372372
ArrayBufferAllocator* allocator,
373373
const EmbedderSnapshotData* embedder_snapshot_data) {
374-
const SnapshotData* snapshot_data =
375-
SnapshotData::FromEmbedderWrapper(embedder_snapshot_data);
376-
return new IsolateData(isolate, loop, platform, allocator, snapshot_data);
374+
return IsolateData::CreateIsolateData(
375+
isolate, loop, platform, allocator, embedder_snapshot_data);
377376
}
378377

379378
void FreeIsolateData(IsolateData* isolate_data) {

‎src/env-inl.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -576,11 +576,6 @@ inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
576576
return options_;
577577
}
578578

579-
inline void IsolateData::set_options(
580-
std::shared_ptr<PerIsolateOptions> options) {
581-
options_ = std::move(options);
582-
}
583-
584579
template <typename Fn>
585580
void Environment::SetImmediate(Fn&& cb, CallbackFlags::Flags flags) {
586581
auto callback = native_immediates_.CreateCallback(std::move(cb), flags);

‎src/env.cc‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -535,19 +535,35 @@ Mutex IsolateData::isolate_data_mutex_;
535535
std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
536536
IsolateData::wrapper_data_map_;
537537

538+
IsolateData* IsolateData::CreateIsolateData(
539+
Isolate* isolate,
540+
uv_loop_t* loop,
541+
MultiIsolatePlatform* platform,
542+
ArrayBufferAllocator* allocator,
543+
const EmbedderSnapshotData* embedder_snapshot_data,
544+
std::shared_ptr<PerIsolateOptions> options) {
545+
const SnapshotData* snapshot_data =
546+
SnapshotData::FromEmbedderWrapper(embedder_snapshot_data);
547+
if (options == nullptr) {
548+
options = per_process::cli_options->per_isolate->Clone();
549+
}
550+
return new IsolateData(
551+
isolate, loop, platform, allocator, snapshot_data, options);
552+
}
553+
538554
IsolateData::IsolateData(Isolate* isolate,
539555
uv_loop_t* event_loop,
540556
MultiIsolatePlatform* platform,
541557
ArrayBufferAllocator* node_allocator,
542-
const SnapshotData* snapshot_data)
558+
const SnapshotData* snapshot_data,
559+
std::shared_ptr<PerIsolateOptions> options)
543560
: isolate_(isolate),
544561
event_loop_(event_loop),
545562
node_allocator_(node_allocator == nullptr ? nullptr
546563
: node_allocator->GetImpl()),
547564
platform_(platform),
548-
snapshot_data_(snapshot_data) {
549-
options_.reset(
550-
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
565+
snapshot_data_(snapshot_data),
566+
options_(std::move(options)) {
551567
v8::CppHeap* cpp_heap = isolate->GetCppHeap();
552568

553569
uint16_t cppgc_id = kDefaultCppGCEmbedderID;

‎src/env.h‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,22 @@ struct PerIsolateWrapperData {
138138
};
139139

140140
class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
141-
public:
141+
private:
142142
IsolateData(v8::Isolate* isolate,
143143
uv_loop_t* event_loop,
144-
MultiIsolatePlatform* platform = nullptr,
145-
ArrayBufferAllocator* node_allocator = nullptr,
146-
const SnapshotData* snapshot_data = nullptr);
144+
MultiIsolatePlatform* platform,
145+
ArrayBufferAllocator* node_allocator,
146+
const SnapshotData* snapshot_data,
147+
std::shared_ptr<PerIsolateOptions> options);
148+
149+
public:
150+
static IsolateData* CreateIsolateData(
151+
v8::Isolate* isolate,
152+
uv_loop_t* event_loop,
153+
MultiIsolatePlatform* platform = nullptr,
154+
ArrayBufferAllocator* node_allocator = nullptr,
155+
const EmbedderSnapshotData* embedder_snapshot_data = nullptr,
156+
std::shared_ptr<PerIsolateOptions> options = nullptr);
147157
~IsolateData();
148158

149159
SET_MEMORY_INFO_NAME(IsolateData)
@@ -172,7 +182,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
172182
inline MultiIsolatePlatform* platform() const;
173183
inline const SnapshotData* snapshot_data() const;
174184
inline std::shared_ptr<PerIsolateOptions> options();
175-
inline void set_options(std::shared_ptr<PerIsolateOptions> options);
176185

177186
inline NodeArrayBufferAllocator* node_allocator() const;
178187

‎src/node_options-inl.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@ EnvironmentOptions* PerIsolateOptions::get_per_env_options() {
1717
return per_env.get();
1818
}
1919

20+
std::shared_ptr<PerIsolateOptions> PerIsolateOptions::Clone() const {
21+
auto options =
22+
std::shared_ptr<PerIsolateOptions>(new PerIsolateOptions(*this));
23+
options->per_env = std::make_shared<EnvironmentOptions>(*per_env);
24+
return options;
25+
}
26+
2027
namespace options_parser {
2128

2229
template <typename Options>

‎src/node_options.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,11 @@ class DebugOptions : public Options {
9494
break_first_line = true;
9595
}
9696

97+
void DisableWaitOrBreakFirstLine() {
98+
inspect_wait = false;
99+
break_first_line = false;
100+
}
101+
97102
bool wait_for_connect() const {
98103
return break_first_line || break_node_first_line || inspect_wait;
99104
}
@@ -245,6 +250,9 @@ class EnvironmentOptions : public Options {
245250

246251
class PerIsolateOptions : public Options {
247252
public:
253+
PerIsolateOptions() = default;
254+
PerIsolateOptions(PerIsolateOptions&&) = default;
255+
248256
std::shared_ptr<EnvironmentOptions> per_env { new EnvironmentOptions() };
249257
bool track_heap_objects = false;
250258
bool report_uncaught_exception = false;
@@ -256,6 +264,11 @@ class PerIsolateOptions : public Options {
256264
inline EnvironmentOptions* get_per_env_options();
257265
void CheckOptions(std::vector<std::string>* errors,
258266
std::vector<std::string>* argv) override;
267+
268+
inline std::shared_ptr<PerIsolateOptions> Clone() const;
269+
270+
private:
271+
PerIsolateOptions(const PerIsolateOptions&) = default;
259272
};
260273

261274
class PerProcessOptions : public Options {

‎src/node_worker.cc‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -185,16 +185,15 @@ class WorkerThreadData {
185185
isolate->SetStackLimit(w->stack_base_);
186186

187187
HandleScope handle_scope(isolate);
188-
isolate_data_.reset(
189-
CreateIsolateData(isolate,
190-
&loop_,
191-
w_->platform_,
192-
allocator.get(),
193-
w->snapshot_data()->AsEmbedderWrapper().get()));
188+
isolate_data_.reset(IsolateData::CreateIsolateData(
189+
isolate,
190+
&loop_,
191+
w_->platform_,
192+
allocator.get(),
193+
w->snapshot_data()->AsEmbedderWrapper().get(),
194+
std::move(w_->per_isolate_opts_)));
194195
CHECK(isolate_data_);
195196
CHECK(!isolate_data_->is_building_snapshot());
196-
if (w_->per_isolate_opts_)
197-
isolate_data_->set_options(std::move(w_->per_isolate_opts_));
198197
isolate_data_->set_worker_context(w_);
199198
isolate_data_->max_young_gen_size =
200199
params.constraints.max_young_generation_size_in_bytes();
@@ -491,9 +490,8 @@ Worker::~Worker() {
491490

492491
void Worker::New(const FunctionCallbackInfo<Value>& args) {
493492
Environment* env = Environment::GetCurrent(args);
494-
auto is_internal = args[5];
495-
CHECK(is_internal->IsBoolean());
496-
if (is_internal->IsFalse()) {
493+
bool is_internal = args[5]->IsTrue();
494+
if (!is_internal) {
497495
THROW_IF_INSUFFICIENT_PERMISSIONS(
498496
env, permission::PermissionScope::kWorkerThreads, "");
499497
}
@@ -658,7 +656,19 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
658656
return;
659657
}
660658
} else {
659+
// Copy the parent's execArgv.
661660
exec_argv_out = env->exec_argv();
661+
per_isolate_opts = env->isolate_data()->options()->Clone();
662+
}
663+
664+
// Internal workers should not wait for inspector frontend to connect or
665+
// break on the first line of internal scripts. Module loader threads are
666+
// essential to load user codes and must not be blocked by the inspector
667+
// for internal scripts.
668+
// Still, `--inspect-node` can break on the first line of internal scripts.
669+
if (is_internal) {
670+
per_isolate_opts->per_env->get_debug_options()
671+
->DisableWaitOrBreakFirstLine();
662672
}
663673

664674
const SnapshotData* snapshot_data = env->isolate_data()->snapshot_data();

‎test/common/inspector-helper.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,10 @@ class InspectorSession {
309309
return notification.method === 'Runtime.executionContextDestroyed' &&
310310
notification.params.executionContextId === 1;
311311
});
312+
await this.waitForDisconnect();
313+
}
314+
315+
async waitForDisconnect() {
312316
while ((await this._instance.nextStderrString()) !==
313317
'Waiting for the debugger to disconnect...');
314318
await this.disconnect();

‎test/parallel/test-esm-loader-hooks-inspect-brk.js‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// This tests esm loader's internal worker will not be blocked by --inspect-brk.
2+
// Regression: https://github.com/nodejs/node/issues/53681
3+
4+
'use strict';
5+
const common = require('../common');
6+
7+
common.skipIfInspectorDisabled();
8+
9+
const assert = require('assert');
10+
const fixtures = require('../common/fixtures');
11+
const { NodeInstance } = require('../common/inspector-helper.js');
12+
13+
async function runIfWaitingForDebugger(session) {
14+
const commands = [
15+
{ 'method': 'Runtime.enable' },
16+
{ 'method': 'Debugger.enable' },
17+
{ 'method': 'Runtime.runIfWaitingForDebugger' },
18+
];
19+
20+
await session.send(commands);
21+
await session.waitForNotification('Debugger.paused');
22+
}
23+
24+
async function runTest() {
25+
const main = fixtures.path('es-module-loaders', 'register-loader.mjs');
26+
const child = new NodeInstance(['--inspect-brk=0'], '', main);
27+
28+
const session = await child.connectInspectorSession();
29+
await runIfWaitingForDebugger(session);
30+
await session.runToCompletion();
31+
assert.strictEqual((await child.expectShutdown()).exitCode, 0);
32+
}
33+
34+
runTest();

‎test/parallel/test-esm-loader-hooks-inspect-wait.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// This tests esm loader's internal worker will not be blocked by --inspect-wait.
2+
// Regression: https://github.com/nodejs/node/issues/53681
3+
4+
'use strict';
5+
const common = require('../common');
6+
7+
common.skipIfInspectorDisabled();
8+
9+
const assert = require('assert');
10+
const fixtures = require('../common/fixtures');
11+
const { NodeInstance } = require('../common/inspector-helper.js');
12+
13+
async function runIfWaitingForDebugger(session) {
14+
const commands = [
15+
{ 'method': 'Runtime.enable' },
16+
{ 'method': 'Debugger.enable' },
17+
{ 'method': 'Runtime.runIfWaitingForDebugger' },
18+
];
19+
20+
await session.send(commands);
21+
}
22+
23+
async function runTest() {
24+
const main = fixtures.path('es-module-loaders', 'register-loader.mjs');
25+
const child = new NodeInstance(['--inspect-wait=0'], '', main);
26+
27+
const session = await child.connectInspectorSession();
28+
await runIfWaitingForDebugger(session);
29+
await session.waitForDisconnect();
30+
assert.strictEqual((await child.expectShutdown()).exitCode, 0);
31+
}
32+
33+
runTest();

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL