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

src: fix Worker termination when '--inspect-brk' is passed · nodejs/node@ef5dabd · GitHub

/ node Public

Commit ef5dabd

Browse files
daeyeon
authored and
marco-ippolito
committed
src: fix Worker termination when '--inspect-brk' is passed
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com> PR-URL: #53724 Fixes: #53648 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
1 parent 4647e6b commit ef5dabd

9 files changed

Lines changed: 114 additions & 20 deletions

File tree

‎src/debug_utils.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ void NODE_EXTERN_PRIVATE FWrite(FILE* file, const std::string& str);
4545
V(DIAGNOSTICS) \
4646
V(HUGEPAGES) \
4747
V(INSPECTOR_SERVER) \
48+
V(INSPECTOR_CLIENT) \
4849
V(INSPECTOR_PROFILER) \
4950
V(CODE_CACHE) \
5051
V(NGTCP2_DEBUG) \

‎src/env-inl.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,10 @@ inline bool Environment::should_create_inspector() const {
664664
!options_->test_runner && !options_->watch_mode;
665665
}
666666

667+
inline bool Environment::should_wait_for_inspector_frontend() const {
668+
return (flags_ & EnvironmentFlags::kNoWaitForInspectorFrontend) == 0;
669+
}
670+
667671
inline bool Environment::tracks_unmanaged_fds() const {
668672
return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
669673
}

‎src/env.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,7 @@ class Environment : public MemoryRetainer {
628628
// the ownership if transferred into the Environment.
629629
void InitializeInspector(
630630
std::unique_ptr<inspector::ParentInspectorHandle> parent_handle);
631+
void WaitForInspectorFrontendByOptions();
631632
#endif
632633

633634
inline size_t async_callback_scope_depth() const;
@@ -798,6 +799,7 @@ class Environment : public MemoryRetainer {
798799
inline bool no_native_addons() const;
799800
inline bool should_not_register_esm_loader() const;
800801
inline bool should_create_inspector() const;
802+
inline bool should_wait_for_inspector_frontend() const;
801803
inline bool owns_process_state() const;
802804
inline bool owns_inspector() const;
803805
inline bool tracks_unmanaged_fds() const;

‎src/inspector_agent.cc‎

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,8 @@ class NodeInspectorClient : public V8InspectorClient {
487487
}
488488

489489
if (interface_) {
490+
per_process::Debug(DebugCategory::INSPECTOR_CLIENT,
491+
"Stopping waiting for frontend events\n");
490492
interface_->StopWaitingForFrontendEvent();
491493
}
492494
}
@@ -668,11 +670,16 @@ class NodeInspectorClient : public V8InspectorClient {
668670

669671
running_nested_loop_ = true;
670672

673+
per_process::Debug(DebugCategory::INSPECTOR_CLIENT,
674+
"Entering nested loop\n");
675+
671676
while (shouldRunMessageLoop()) {
672677
if (interface_) interface_->WaitForFrontendEvent();
673678
env_->RunAndClearInterrupts();
674679
}
675680
running_nested_loop_ = false;
681+
682+
per_process::Debug(DebugCategory::INSPECTOR_CLIENT, "Exited nested loop\n");
676683
}
677684

678685
double currentTimeMS() override {
@@ -759,27 +766,11 @@ bool Agent::Start(const std::string& path,
759766
}
760767
}, parent_env_);
761768

762-
bool wait_for_connect = options.wait_for_connect();
763-
bool should_break_first_line = options.should_break_first_line();
764-
if (parent_handle_) {
765-
should_break_first_line = parent_handle_->WaitForConnect();
766-
parent_handle_->WorkerStarted(client_->getThreadHandle(),
767-
should_break_first_line);
768-
} else if (!options.inspector_enabled || !options.allow_attaching_debugger ||
769-
!StartIoThread()) {
769+
if (!parent_handle_ &&
770+
(!options.inspector_enabled || !options.allow_attaching_debugger ||
771+
!StartIoThread())) {
770772
return false;
771773
}
772-
773-
if (wait_for_connect || should_break_first_line) {
774-
// Patch the debug options to implement waitForDebuggerOnStart for
775-
// the NodeWorker.enable method.
776-
if (should_break_first_line) {
777-
CHECK(!parent_env_->has_serialized_options());
778-
debug_options_.EnableBreakFirstLine();
779-
parent_env_->options()->get_debug_options()->EnableBreakFirstLine();
780-
}
781-
client_->waitForFrontend();
782-
}
783774
return true;
784775
}
785776

@@ -1038,6 +1029,33 @@ void Agent::WaitForConnect() {
10381029
client_->waitForFrontend();
10391030
}
10401031

1032+
bool Agent::WaitForConnectByOptions() {
1033+
if (client_ == nullptr) {
1034+
return false;
1035+
}
1036+
1037+
bool wait_for_connect = debug_options_.wait_for_connect();
1038+
bool should_break_first_line = debug_options_.should_break_first_line();
1039+
if (parent_handle_) {
1040+
should_break_first_line = parent_handle_->WaitForConnect();
1041+
parent_handle_->WorkerStarted(client_->getThreadHandle(),
1042+
should_break_first_line);
1043+
}
1044+
1045+
if (wait_for_connect || should_break_first_line) {
1046+
// Patch the debug options to implement waitForDebuggerOnStart for
1047+
// the NodeWorker.enable method.
1048+
if (should_break_first_line) {
1049+
CHECK(!parent_env_->has_serialized_options());
1050+
debug_options_.EnableBreakFirstLine();
1051+
parent_env_->options()->get_debug_options()->EnableBreakFirstLine();
1052+
}
1053+
client_->waitForFrontend();
1054+
return true;
1055+
}
1056+
return false;
1057+
}
1058+
10411059
void Agent::StopIfWaitingForConnect() {
10421060
if (client_ == nullptr) {
10431061
return;

‎src/inspector_agent.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ class Agent {
6161

6262
// Blocks till frontend connects and sends "runIfWaitingForDebugger"
6363
void WaitForConnect();
64+
bool WaitForConnectByOptions();
6465
void StopIfWaitingForConnect();
6566

6667
// Blocks till all the sessions with "WaitForDisconnectOnShutdown" disconnect

‎src/node.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,17 @@ void Environment::InitializeInspector(
204204
return;
205205
}
206206

207+
if (should_wait_for_inspector_frontend()) {
208+
WaitForInspectorFrontendByOptions();
209+
}
210+
207211
profiler::StartProfilers(this);
212+
}
213+
214+
void Environment::WaitForInspectorFrontendByOptions() {
215+
if (!inspector_agent_->WaitForConnectByOptions()) {
216+
return;
217+
}
208218

209219
if (inspector_agent_->options().break_node_first_line) {
210220
inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap");

‎src/node.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -661,7 +661,11 @@ enum Flags : uint64_t {
661661
// Controls where or not the InspectorAgent for this Environment should
662662
// call StartDebugSignalHandler. This control is needed by embedders who may
663663
// not want to allow other processes to start the V8 inspector.
664-
kNoStartDebugSignalHandler = 1 << 10
664+
kNoStartDebugSignalHandler = 1 << 10,
665+
// Controls whether the InspectorAgent created for this Environment waits for
666+
// Inspector frontend events during the Environment creation. It's used to
667+
// call node::Stop(env) on a Worker thread that is waiting for the events.
668+
kNoWaitForInspectorFrontend = 1 << 11
665669
};
666670
} // namespace EnvironmentFlags
667671

‎src/node_worker.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,9 @@ void Worker::Run() {
359359
CHECK(!context.IsEmpty());
360360
Context::Scope context_scope(context);
361361
{
362+
#if HAVE_INSPECTOR
363+
environment_flags_ |= EnvironmentFlags::kNoWaitForInspectorFrontend;
364+
#endif
362365
env_.reset(CreateEnvironment(
363366
data.isolate_data_.get(),
364367
context,
@@ -380,6 +383,10 @@ void Worker::Run() {
380383
this->env_ = env_.get();
381384
}
382385
Debug(this, "Created Environment for worker with id %llu", thread_id_.id);
386+
387+
#if HAVE_INSPECTOR
388+
this->env_->WaitForInspectorFrontendByOptions();
389+
#endif
383390
if (is_stopped()) return;
384391
{
385392
if (!CreateEnvMessagePort(env_.get())) {

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
common.skipIfInspectorDisabled();
5+
6+
const { workerData, Worker } = require('node:worker_threads');
7+
if (!workerData) {
8+
common.skipIfWorker();
9+
}
10+
11+
const assert = require('node:assert');
12+
13+
let TIMEOUT = common.platformTimeout(5000);
14+
if (common.isWindows) {
15+
// Refs: https://github.com/nodejs/build/issues/3014
16+
TIMEOUT = common.platformTimeout(15000);
17+
}
18+
19+
// Refs: https://github.com/nodejs/node/issues/53648
20+
21+
(async () => {
22+
if (!workerData) {
23+
// worker.terminate() should terminate the worker created with execArgv:
24+
// ["--inspect-brk"].
25+
{
26+
const worker = new Worker(__filename, {
27+
execArgv: ['--inspect-brk=0'],
28+
workerData: {},
29+
});
30+
await new Promise((r) => setTimeout(r, TIMEOUT));
31+
worker.on('exit', common.mustCall());
32+
await worker.terminate();
33+
}
34+
// process.exit() should kill the process.
35+
{
36+
new Worker(__filename, {
37+
execArgv: ['--inspect-brk=0'],
38+
workerData: {},
39+
});
40+
await new Promise((r) => setTimeout(r, TIMEOUT));
41+
process.on('exit', (status) => assert.strictEqual(status, 0));
42+
setImmediate(() => process.exit());
43+
}
44+
} else {
45+
console.log('Worker running!');
46+
}
47+
})().then(common.mustCall());

0 commit comments

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