Skip to content

Commit 3fe59ba

Browse files
mmarchinirichardlau
mmarchini
authored and
richardlau
committed
inspector: add NodeRuntime.waitingForDebugger event
`NodeRuntime.waitingForDebugger` is a new Inspector Protocol event that will fire when the process being inspected is waiting for the debugger (for example, when `inspector.waitForDebugger()` is called). This allows inspecting processes to know when the inspected process is waiting for a `Runtime.runIfWaitingForDebugger` message to resume execution. It allows tooling to resume execution of the inspected process as soon as it deems necessary, without having to guess if the inspected process is waiting or not, making the workflow more deterministic. With a more deterministic workflow, it is possible to update Node.js core tests to avoid race conditions that can cause flakiness. Therefore, tests were also changed as following: * Remove no-op Runtime.runIfWaitingForDebugger from tests that don't need it * Use NodeRuntime.waitingForDebugger in all tests that need Runtime.runIfWaitingForDebugger, to ensure order of operations is predictable and correct * Simplify test-inspector-multisession-ws There might be value in adding `NodeWorker.waitingForDebugger` in a future patch, but as of right now, no Node.js core inspector tests using worker threads are not failing due to race conditions. Fixes: #34730 PR-URL: #51560 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
1 parent 06a29f8 commit 3fe59ba

25 files changed

+160
-28
lines changed

‎src/inspector/node_protocol.pdl

+10
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,12 @@ experimental domain NodeWorker
100100

101101
# Support for inspecting node process state.
102102
experimental domain NodeRuntime
103+
# Enable the NodeRuntime events except by `NodeRuntime.waitingForDisconnect`.
104+
command enable
105+
106+
# Disable NodeRuntime events
107+
command disable
108+
103109
# Enable the `NodeRuntime.waitingForDisconnect`.
104110
command notifyWhenWaitingForDisconnect
105111
parameters
@@ -110,3 +116,7 @@ experimental domain NodeRuntime
110116
# It is fired when the Node process finished all code execution and is
111117
# waiting for all frontends to disconnect.
112118
event waitingForDisconnect
119+
120+
# This event is fired when the runtime is waiting for the debugger. For
121+
# example, when inspector.waitingForDebugger is called
122+
event waitingForDebugger

‎src/inspector/runtime_agent.cc

+27-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ namespace inspector {
88
namespace protocol {
99

1010
RuntimeAgent::RuntimeAgent()
11-
: notify_when_waiting_for_disconnect_(false) {}
11+
: notify_when_waiting_for_disconnect_(false),
12+
enabled_(false),
13+
is_waiting_for_debugger_(false) {}
1214

1315
void RuntimeAgent::Wire(UberDispatcher* dispatcher) {
1416
frontend_ = std::make_unique<NodeRuntime::Frontend>(dispatcher->channel());
@@ -20,6 +22,30 @@ DispatchResponse RuntimeAgent::notifyWhenWaitingForDisconnect(bool enabled) {
2022
return DispatchResponse::OK();
2123
}
2224

25+
DispatchResponse RuntimeAgent::enable() {
26+
enabled_ = true;
27+
if (is_waiting_for_debugger_) {
28+
frontend_->waitingForDebugger();
29+
}
30+
return DispatchResponse::OK();
31+
}
32+
33+
DispatchResponse RuntimeAgent::disable() {
34+
enabled_ = false;
35+
return DispatchResponse::OK();
36+
}
37+
38+
void RuntimeAgent::setWaitingForDebugger() {
39+
is_waiting_for_debugger_ = true;
40+
if (enabled_) {
41+
frontend_->waitingForDebugger();
42+
}
43+
}
44+
45+
void RuntimeAgent::unsetWaitingForDebugger() {
46+
is_waiting_for_debugger_ = false;
47+
}
48+
2349
bool RuntimeAgent::notifyWaitingForDisconnect() {
2450
if (notify_when_waiting_for_disconnect_) {
2551
frontend_->waitingForDisconnect();

‎src/inspector/runtime_agent.h

+10
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,21 @@ class RuntimeAgent : public NodeRuntime::Backend {
1818

1919
DispatchResponse notifyWhenWaitingForDisconnect(bool enabled) override;
2020

21+
DispatchResponse enable() override;
22+
23+
DispatchResponse disable() override;
24+
2125
bool notifyWaitingForDisconnect();
2226

27+
void setWaitingForDebugger();
28+
29+
void unsetWaitingForDebugger();
30+
2331
private:
2432
std::shared_ptr<NodeRuntime::Frontend> frontend_;
2533
bool notify_when_waiting_for_disconnect_;
34+
bool enabled_;
35+
bool is_waiting_for_debugger_;
2636
};
2737
} // namespace protocol
2838
} // namespace inspector

‎src/inspector_agent.cc

+13
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
278278
return retaining_context_;
279279
}
280280

281+
void setWaitingForDebugger() { runtime_agent_->setWaitingForDebugger(); }
282+
283+
void unsetWaitingForDebugger() { runtime_agent_->unsetWaitingForDebugger(); }
284+
281285
bool retainingContext() {
282286
return retaining_context_;
283287
}
@@ -418,6 +422,9 @@ class NodeInspectorClient : public V8InspectorClient {
418422

419423
void waitForFrontend() {
420424
waiting_for_frontend_ = true;
425+
for (const auto& id_channel : channels_) {
426+
id_channel.second->setWaitingForDebugger();
427+
}
421428
runMessageLoop();
422429
}
423430

@@ -465,6 +472,9 @@ class NodeInspectorClient : public V8InspectorClient {
465472

466473
void runIfWaitingForDebugger(int context_group_id) override {
467474
waiting_for_frontend_ = false;
475+
for (const auto& id_channel : channels_) {
476+
id_channel.second->unsetWaitingForDebugger();
477+
}
468478
}
469479

470480
int connectFrontend(std::unique_ptr<InspectorSessionDelegate> delegate,
@@ -476,6 +486,9 @@ class NodeInspectorClient : public V8InspectorClient {
476486
std::move(delegate),
477487
getThreadHandle(),
478488
prevent_shutdown);
489+
if (waiting_for_frontend_) {
490+
channels_[session_id]->setWaitingForDebugger();
491+
}
479492
return session_id;
480493
}
481494

‎test/common/inspector-helper.js

+3
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,9 @@ class InspectorSession {
303303
console.log('[test]', 'Verify node waits for the frontend to disconnect');
304304
await this.send({ 'method': 'Debugger.resume' });
305305
await this.waitForNotification((notification) => {
306+
if (notification.method === 'Debugger.paused') {
307+
this.send({ 'method': 'Debugger.resume' });
308+
}
306309
return notification.method === 'Runtime.executionContextDestroyed' &&
307310
notification.params.executionContextId === 1;
308311
});

‎test/parallel/test-inspect-async-hook-setup-at-inspect.js

-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ async function runTests() {
5454
'params': { 'maxDepth': 10 } },
5555
{ 'method': 'Debugger.setBlackboxPatterns',
5656
'params': { 'patterns': [] } },
57-
{ 'method': 'Runtime.runIfWaitingForDebugger' },
5857
]);
5958

6059
await waitForInitialSetup(session);

‎test/parallel/test-inspector-async-hook-setup-at-inspect-brk.js

+3
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ async function checkAsyncStackTrace(session) {
3030
async function runTests() {
3131
const instance = new NodeInstance(undefined, script);
3232
const session = await instance.connectInspectorSession();
33+
await session.send({ method: 'NodeRuntime.enable' });
34+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
3335
await session.send([
3436
{ 'method': 'Runtime.enable' },
3537
{ 'method': 'Debugger.enable' },
@@ -39,6 +41,7 @@ async function runTests() {
3941
'params': { 'patterns': [] } },
4042
{ 'method': 'Runtime.runIfWaitingForDebugger' },
4143
]);
44+
await session.send({ method: 'NodeRuntime.disable' });
4245
await skipBreakpointAtStart(session);
4346
await checkAsyncStackTrace(session);
4447

‎test/parallel/test-inspector-async-hook-setup-at-signal.js

-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ async function runTests() {
6969
'params': { 'maxDepth': 10 } },
7070
{ 'method': 'Debugger.setBlackboxPatterns',
7171
'params': { 'patterns': [] } },
72-
{ 'method': 'Runtime.runIfWaitingForDebugger' },
7372
]);
7473

7574
await waitForInitialSetup(session);

‎test/parallel/test-inspector-async-stack-traces-promise-then.js

+3
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ function runTest() {
2020
async function runTests() {
2121
const instance = new NodeInstance(undefined, script);
2222
const session = await instance.connectInspectorSession();
23+
await session.send({ method: 'NodeRuntime.enable' });
24+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
2325
await session.send([
2426
{ 'method': 'Runtime.enable' },
2527
{ 'method': 'Debugger.enable' },
@@ -29,6 +31,7 @@ async function runTests() {
2931
'params': { 'patterns': [] } },
3032
{ 'method': 'Runtime.runIfWaitingForDebugger' },
3133
]);
34+
session.send({ method: 'NodeRuntime.disable' });
3235

3336
await session.waitForBreakOnLine(0, '[eval]');
3437
await session.send({ 'method': 'Debugger.resume' });

‎test/parallel/test-inspector-async-stack-traces-set-interval.js

+3
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ async function checkAsyncStackTrace(session) {
2626
async function runTests() {
2727
const instance = new NodeInstance(undefined, script);
2828
const session = await instance.connectInspectorSession();
29+
await session.send({ method: 'NodeRuntime.enable' });
30+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
2931
await session.send([
3032
{ 'method': 'Runtime.enable' },
3133
{ 'method': 'Debugger.enable' },
@@ -35,6 +37,7 @@ async function runTests() {
3537
'params': { 'patterns': [] } },
3638
{ 'method': 'Runtime.runIfWaitingForDebugger' },
3739
]);
40+
await session.send({ method: 'NodeRuntime.disable' });
3841

3942
await skipFirstBreakpoint(session);
4043
await checkAsyncStackTrace(session);

‎test/parallel/test-inspector-break-e.js

+3
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@ const { NodeInstance } = require('../common/inspector-helper.js');
88
async function runTests() {
99
const instance = new NodeInstance(undefined, 'console.log(10)');
1010
const session = await instance.connectInspectorSession();
11+
await session.send({ method: 'NodeRuntime.enable' });
12+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
1113
await session.send([
1214
{ 'method': 'Runtime.enable' },
1315
{ 'method': 'Debugger.enable' },
1416
{ 'method': 'Runtime.runIfWaitingForDebugger' },
1517
]);
18+
await session.send({ method: 'NodeRuntime.disable' });
1619
await session.waitForBreakOnLine(0, '[eval]');
1720
await session.runToCompletion();
1821
assert.strictEqual((await instance.expectShutdown()).exitCode, 0);

‎test/parallel/test-inspector-break-when-eval.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ async function setupDebugger(session) {
2020
'params': { 'maxDepth': 0 } },
2121
{ 'method': 'Runtime.runIfWaitingForDebugger' },
2222
];
23-
session.send(commands);
23+
await session.send({ method: 'NodeRuntime.enable' });
24+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
25+
await session.send(commands);
26+
await session.send({ method: 'NodeRuntime.disable' });
2427

2528
await session.waitForNotification('Debugger.paused', 'Initial pause');
2629

‎test/parallel/test-inspector-console.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ async function runTest() {
2020
{ 'method': 'Runtime.runIfWaitingForDebugger' },
2121
];
2222

23-
session.send(commands);
23+
await session.send({ method: 'NodeRuntime.enable' });
24+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
25+
await session.send(commands);
26+
await session.send({ method: 'NodeRuntime.disable' });
2427

2528
const msg = await session.waitForNotification('Runtime.consoleAPICalled');
2629

‎test/parallel/test-inspector-debug-brk-flag.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ async function testBreakpointOnStart(session) {
2222
{ 'method': 'Runtime.runIfWaitingForDebugger' },
2323
];
2424

25-
session.send(commands);
25+
await session.send({ method: 'NodeRuntime.enable' });
26+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
27+
await session.send(commands);
28+
await session.send({ method: 'NodeRuntime.disable' });
2629
await session.waitForBreakOnLine(0, session.scriptURL());
2730
}
2831

‎test/parallel/test-inspector-debug-end.js

+2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ async function testSessionNoCrash() {
3030

3131
const instance = new NodeInstance('--inspect-brk=0', script);
3232
const session = await instance.connectInspectorSession();
33+
await session.send({ method: 'NodeRuntime.enable' });
34+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
3335
await session.send({ 'method': 'Runtime.runIfWaitingForDebugger' });
3436
await session.waitForServerDisconnect();
3537
strictEqual((await instance.expectShutdown()).exitCode, 42);

‎test/parallel/test-inspector-esm.js

+3
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,10 @@ async function testBreakpointOnStart(session) {
3636
{ 'method': 'Runtime.runIfWaitingForDebugger' },
3737
];
3838

39+
await session.send({ method: 'NodeRuntime.enable' });
40+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
3941
await session.send(commands);
42+
await session.send({ method: 'NodeRuntime.disable' });
4043
await session.waitForBreakOnLine(
4144
0, UrlResolve(session.scriptURL().toString(), 'message.mjs'));
4245
}

‎test/parallel/test-inspector-exception.js

+3
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ async function testBreakpointOnStart(session) {
2828
{ 'method': 'Runtime.runIfWaitingForDebugger' },
2929
];
3030

31+
await session.send({ method: 'NodeRuntime.enable' });
32+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
3133
await session.send(commands);
34+
await session.send({ method: 'NodeRuntime.disable' });
3235
await session.waitForBreakOnLine(21, pathToFileURL(script).toString());
3336
}
3437

‎test/parallel/test-inspector-inspect-brk-node.js

+3
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ const { NodeInstance } = require('../common/inspector-helper.js');
1010
async function runTest() {
1111
const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']);
1212
const session = await child.connectInspectorSession();
13+
await session.send({ method: 'NodeRuntime.enable' });
14+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
1315
await session.send({ method: 'Runtime.enable' });
1416
await session.send({ method: 'Debugger.enable' });
1517
await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
18+
await session.send({ method: 'NodeRuntime.disable' });
1619
await session.waitForNotification((notification) => {
1720
// The main assertion here is that we do hit the loader script first.
1821
return notification.method === 'Debugger.scriptParsed' &&

‎test/parallel/test-inspector-multisession-ws.js

+15-7
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,12 @@ session.on('Debugger.paused', () => {
2020
session.connect();
2121
session.post('Debugger.enable');
2222
console.log('Ready');
23-
console.log('Ready');
2423
`;
2524

2625
async function setupSession(node) {
2726
const session = await node.connectInspectorSession();
27+
await session.send({ method: 'NodeRuntime.enable' });
28+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
2829
await session.send([
2930
{ 'method': 'Runtime.enable' },
3031
{ 'method': 'Debugger.enable' },
@@ -37,20 +38,19 @@ async function setupSession(node) {
3738
'params': { 'interval': 100 } },
3839
{ 'method': 'Debugger.setBlackboxPatterns',
3940
'params': { 'patterns': [] } },
40-
{ 'method': 'Runtime.runIfWaitingForDebugger' },
4141
]);
42+
4243
return session;
4344
}
4445

4546
async function testSuspend(sessionA, sessionB) {
4647
console.log('[test]', 'Breaking in code and verifying events are fired');
47-
await sessionA.waitForNotification('Debugger.paused', 'Initial pause');
48+
await Promise.all([
49+
sessionA.waitForNotification('Debugger.paused', 'Initial sessionA paused'),
50+
sessionB.waitForNotification('Debugger.paused', 'Initial sessionB paused'),
51+
]);
4852
sessionA.send({ 'method': 'Debugger.resume' });
4953

50-
await sessionA.waitForNotification('Runtime.consoleAPICalled',
51-
'Console output');
52-
// NOTE(mmarchini): Remove second console.log when
53-
// https://bugs.chromium.org/p/v8/issues/detail?id=10287 is fixed.
5454
await sessionA.waitForNotification('Runtime.consoleAPICalled',
5555
'Console output');
5656
sessionA.send({ 'method': 'Debugger.pause' });
@@ -65,6 +65,14 @@ async function runTest() {
6565

6666
const [session1, session2] =
6767
await Promise.all([setupSession(child), setupSession(child)]);
68+
await Promise.all([
69+
session1.send({ method: 'Runtime.runIfWaitingForDebugger' }),
70+
session2.send({ method: 'Runtime.runIfWaitingForDebugger' }),
71+
]);
72+
await Promise.all([
73+
session1.send({ method: 'NodeRuntime.disable' }),
74+
session2.send({ method: 'NodeRuntime.disable' }),
75+
]);
6876
await testSuspend(session2, session1);
6977
console.log('[test]', 'Should shut down after both sessions disconnect');
7078

‎test/parallel/test-inspector-scriptparsed-context.js

+7-4
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,13 @@ async function runTests() {
4646
const instance = new NodeInstance(['--inspect-brk=0', '--expose-internals'],
4747
script);
4848
const session = await instance.connectInspectorSession();
49-
await session.send([
50-
{ 'method': 'Debugger.enable' },
51-
{ 'method': 'Runtime.runIfWaitingForDebugger' },
52-
]);
49+
50+
await session.send({ method: 'NodeRuntime.enable' });
51+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
52+
await session.send({ 'method': 'Debugger.enable' });
53+
await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
54+
await session.send({ method: 'NodeRuntime.disable' });
55+
5356
await session.waitForBreakOnLine(2, '[eval]');
5457

5558
await session.send({ 'method': 'Runtime.enable' });

‎test/parallel/test-inspector-stop-profile-after-done.js

+7-4
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,15 @@ async function runTests() {
1414
}, ${common.platformTimeout(30)});`);
1515
const session = await child.connectInspectorSession();
1616

17-
session.send([
17+
await session.send({ method: 'NodeRuntime.enable' });
18+
await session.waitForNotification('NodeRuntime.waitingForDebugger');
19+
await session.send([
1820
{ method: 'Profiler.setSamplingInterval',
1921
params: { interval: common.platformTimeout(300) } },
20-
{ method: 'Profiler.enable' },
21-
{ method: 'Runtime.runIfWaitingForDebugger' },
22-
{ method: 'Profiler.start' }]);
22+
{ method: 'Profiler.enable' }]);
23+
await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
24+
await session.send({ method: 'NodeRuntime.disable' });
25+
await session.send({ method: 'Profiler.start' });
2326
while (await child.nextStderrString() !==
2427
'Waiting for the debugger to disconnect...');
2528
await session.send({ method: 'Profiler.stop' });

0 commit comments

Comments
 (0)