Skip to content

Commit 5886e20

Browse files
ofrobotsofrobots
committed
inspector: track async stacks when necessary
With this change, we do async stack tracking only when explicitly requested by the inspector client. This avoids unnecessary overhead for clients that might not be interested in async stack traces. PR-URL: #16308 Fixes: #16180 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
1 parent c087502 commit 5886e20

File tree

6 files changed

+142
-69
lines changed

6 files changed

+142
-69
lines changed

‎lib/internal/inspector_async_hook.js‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,4 @@ function disable() {
5555

5656
exports.setup = function() {
5757
inspector.registerAsyncHook(enable, disable);
58-
59-
if (inspector.isEnabled()) {
60-
// If the inspector was already enabled via --inspect or --inspect-brk,
61-
// the we need to enable the async hook immediately at startup.
62-
enable();
63-
}
6458
};

‎src/inspector_agent.cc‎

Lines changed: 55 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,14 @@ class NodeInspectorClient : public V8InspectorClient {
319319
return uv_hrtime() * 1.0 / NANOS_PER_MSEC;
320320
}
321321

322+
void maxAsyncCallStackDepthChanged(int depth) override {
323+
if (depth == 0) {
324+
env_->inspector_agent()->DisableAsyncHook();
325+
} else {
326+
env_->inspector_agent()->EnableAsyncHook();
327+
}
328+
}
329+
322330
void contextCreated(Local<Context> context, const std::string& name) {
323331
std::unique_ptr<StringBuffer> name_buffer = Utf8ToStringView(name);
324332
v8_inspector::V8ContextInfo info(context, CONTEXT_GROUP_ID,
@@ -449,7 +457,9 @@ Agent::Agent(Environment* env) : parent_env_(env),
449457
client_(nullptr),
450458
platform_(nullptr),
451459
enabled_(false),
452-
next_context_number_(1) {}
460+
next_context_number_(1),
461+
pending_enable_async_hook_(false),
462+
pending_disable_async_hook_(false) {}
453463

454464
// Destructor needs to be defined here in implementation file as the header
455465
// does not have full definition of some classes.
@@ -498,17 +508,6 @@ bool Agent::StartIoThread(bool wait_for_connect) {
498508
HandleScope handle_scope(isolate);
499509
auto context = parent_env_->context();
500510

501-
// Enable tracking of async stack traces
502-
if (!enable_async_hook_function_.IsEmpty()) {
503-
Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
504-
auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
505-
if (result.IsEmpty()) {
506-
FatalError(
507-
"node::InspectorAgent::StartIoThread",
508-
"Cannot enable Inspector's AsyncHook, please report this.");
509-
}
510-
}
511-
512511
// Send message to enable debug in workers
513512
Local<Object> process_object = parent_env_->process_object();
514513
Local<Value> emit_fn =
@@ -537,38 +536,9 @@ void Agent::Stop() {
537536
io_.reset();
538537
enabled_ = false;
539538
}
540-
541-
v8::Isolate* isolate = parent_env_->isolate();
542-
HandleScope handle_scope(isolate);
543-
544-
// Disable tracking of async stack traces
545-
if (!disable_async_hook_function_.IsEmpty()) {
546-
Local<Function> disable_fn = disable_async_hook_function_.Get(isolate);
547-
auto result = disable_fn->Call(parent_env_->context(),
548-
Undefined(parent_env_->isolate()), 0, nullptr);
549-
if (result.IsEmpty()) {
550-
FatalError(
551-
"node::InspectorAgent::Stop",
552-
"Cannot disable Inspector's AsyncHook, please report this.");
553-
}
554-
}
555539
}
556540

557541
void Agent::Connect(InspectorSessionDelegate* delegate) {
558-
if (!enabled_) {
559-
// Enable tracking of async stack traces
560-
v8::Isolate* isolate = parent_env_->isolate();
561-
HandleScope handle_scope(isolate);
562-
auto context = parent_env_->context();
563-
Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
564-
auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
565-
if (result.IsEmpty()) {
566-
FatalError(
567-
"node::InspectorAgent::Connect",
568-
"Cannot enable Inspector's AsyncHook, please report this.");
569-
}
570-
}
571-
572542
enabled_ = true;
573543
client_->connectFrontend(delegate);
574544
}
@@ -626,6 +596,50 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
626596
v8::Local<v8::Function> disable_function) {
627597
enable_async_hook_function_.Reset(isolate, enable_function);
628598
disable_async_hook_function_.Reset(isolate, disable_function);
599+
if (pending_enable_async_hook_) {
600+
CHECK(!pending_disable_async_hook_);
601+
pending_enable_async_hook_ = false;
602+
EnableAsyncHook();
603+
} else if (pending_disable_async_hook_) {
604+
CHECK(!pending_enable_async_hook_);
605+
pending_disable_async_hook_ = false;
606+
DisableAsyncHook();
607+
}
608+
}
609+
610+
void Agent::EnableAsyncHook() {
611+
if (!enable_async_hook_function_.IsEmpty()) {
612+
Isolate* isolate = parent_env_->isolate();
613+
ToggleAsyncHook(isolate, enable_async_hook_function_.Get(isolate));
614+
} else if (pending_disable_async_hook_) {
615+
CHECK(!pending_enable_async_hook_);
616+
pending_disable_async_hook_ = false;
617+
} else {
618+
pending_enable_async_hook_ = true;
619+
}
620+
}
621+
622+
void Agent::DisableAsyncHook() {
623+
if (!disable_async_hook_function_.IsEmpty()) {
624+
Isolate* isolate = parent_env_->isolate();
625+
ToggleAsyncHook(isolate, disable_async_hook_function_.Get(isolate));
626+
} else if (pending_enable_async_hook_) {
627+
CHECK(!pending_disable_async_hook_);
628+
pending_enable_async_hook_ = false;
629+
} else {
630+
pending_disable_async_hook_ = true;
631+
}
632+
}
633+
634+
void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
635+
HandleScope handle_scope(isolate);
636+
auto context = parent_env_->context();
637+
auto result = fn->Call(context, Undefined(isolate), 0, nullptr);
638+
if (result.IsEmpty()) {
639+
FatalError(
640+
"node::inspector::Agent::ToggleAsyncHook",
641+
"Cannot toggle Inspector's AsyncHook, please report this.");
642+
}
629643
}
630644

631645
void Agent::AsyncTaskScheduled(const StringView& task_name, void* task,

‎src/inspector_agent.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,12 @@ class Agent {
9292
DebugOptions& options() { return debug_options_; }
9393
void ContextCreated(v8::Local<v8::Context> context);
9494

95+
void EnableAsyncHook();
96+
void DisableAsyncHook();
97+
9598
private:
99+
void ToggleAsyncHook(v8::Isolate* isolate, v8::Local<v8::Function> fn);
100+
96101
node::Environment* parent_env_;
97102
std::unique_ptr<NodeInspectorClient> client_;
98103
std::unique_ptr<InspectorIo> io_;
@@ -102,6 +107,8 @@ class Agent {
102107
DebugOptions debug_options_;
103108
int next_context_number_;
104109

110+
bool pending_enable_async_hook_;
111+
bool pending_disable_async_hook_;
105112
v8::Persistent<v8::Function> enable_async_hook_function_;
106113
v8::Persistent<v8::Function> disable_async_hook_function_;
107114
};

‎test/sequential/sequential.status‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ prefix sequential
77
[true] # This section applies to all platforms
88

99
[$system==win32]
10+
test-inspector-async-call-stack : PASS, FLAKY
1011
test-inspector-bindings : PASS, FLAKY
1112
test-inspector-debug-end : PASS, FLAKY
1213
test-inspector-stop-profile-after-done: PASS, FLAKY

‎test/sequential/test-inspector-async-call-stack.js‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
'use strict';
2+
const common = require('../common');
3+
common.skipIfInspectorDisabled();
4+
common.skipIf32Bits();
5+
6+
const assert = require('assert');
7+
const async_wrap = process.binding('async_wrap');
8+
const { kTotals } = async_wrap.constants;
9+
const inspector = require('inspector');
10+
11+
const setDepth = 'Debugger.setAsyncCallStackDepth';
12+
13+
function verifyAsyncHookDisabled(message) {
14+
assert.strictEqual(async_wrap.async_hook_fields[kTotals], 0);
15+
}
16+
17+
function verifyAsyncHookEnabled(message) {
18+
assert.strictEqual(async_wrap.async_hook_fields[kTotals], 4);
19+
}
20+
21+
// By default inspector async hooks should not have been installed.
22+
verifyAsyncHookDisabled('inspector async hook should be disabled at startup');
23+
24+
const session = new inspector.Session();
25+
verifyAsyncHookDisabled('creating a session should not enable async hooks');
26+
27+
session.connect();
28+
verifyAsyncHookDisabled('connecting a session should not enable async hooks');
29+
30+
session.post('Debugger.enable', () => {
31+
verifyAsyncHookDisabled('enabling debugger should not enable async hooks');
32+
33+
session.post(setDepth, { invalid: 'message' }, () => {
34+
verifyAsyncHookDisabled('invalid message should not enable async hooks');
35+
36+
session.post(setDepth, { maxDepth: 'five' }, () => {
37+
verifyAsyncHookDisabled('invalid maxDepth (string) should not enable ' +
38+
'async hooks');
39+
40+
session.post(setDepth, { maxDepth: NaN }, () => {
41+
verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable ' +
42+
'async hooks');
43+
44+
session.post(setDepth, { maxDepth: 10 }, () => {
45+
verifyAsyncHookEnabled('valid message should enable async hooks');
46+
47+
session.post(setDepth, { maxDepth: 0 }, () => {
48+
verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' +
49+
'async hooks');
50+
51+
runTestSet2(session);
52+
});
53+
});
54+
});
55+
});
56+
});
57+
});
58+
59+
function runTestSet2(session) {
60+
session.post(setDepth, { maxDepth: 32 }, () => {
61+
verifyAsyncHookEnabled('valid message should enable async hooks');
62+
63+
session.post('Debugger.disable', () => {
64+
verifyAsyncHookDisabled('Debugger.disable should disable async hooks');
65+
66+
session.post('Debugger.enable', () => {
67+
verifyAsyncHookDisabled('Enabling debugger should not enable hooks');
68+
69+
session.post(setDepth, { maxDepth: 64 }, () => {
70+
verifyAsyncHookEnabled('valid message should enable async hooks');
71+
72+
session.disconnect();
73+
verifyAsyncHookDisabled('Disconnecting session should disable ' +
74+
'async hooks');
75+
});
76+
});
77+
});
78+
});
79+
}

‎test/sequential/test-inspector-async-hook-teardown-at-debug-end.js‎

Lines changed: 0 additions & 22 deletions
This file was deleted.

0 commit comments

Comments
 (0)