Skip to content

Commit

Permalink
inspector: track async stacks when necessary
Browse files Browse the repository at this point in the history
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: nodejs#16308
Fixes: nodejs#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>
  • Loading branch information
ofrobots committed Oct 29, 2017
1 parent c087502 commit 5886e20
Show file tree
Hide file tree
Showing 6 changed files with 142 additions and 69 deletions.
6 changes: 0 additions & 6 deletions lib/internal/inspector_async_hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,4 @@ function disable() {

exports.setup = function() {
inspector.registerAsyncHook(enable, disable);

if (inspector.isEnabled()) {
// If the inspector was already enabled via --inspect or --inspect-brk,
// the we need to enable the async hook immediately at startup.
enable();
}
};
96 changes: 55 additions & 41 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,14 @@ class NodeInspectorClient : public V8InspectorClient {
return uv_hrtime() * 1.0 / NANOS_PER_MSEC;
}

void maxAsyncCallStackDepthChanged(int depth) override {
if (depth == 0) {
env_->inspector_agent()->DisableAsyncHook();
} else {
env_->inspector_agent()->EnableAsyncHook();
}
}

void contextCreated(Local<Context> context, const std::string& name) {
std::unique_ptr<StringBuffer> name_buffer = Utf8ToStringView(name);
v8_inspector::V8ContextInfo info(context, CONTEXT_GROUP_ID,
Expand Down Expand Up @@ -449,7 +457,9 @@ Agent::Agent(Environment* env) : parent_env_(env),
client_(nullptr),
platform_(nullptr),
enabled_(false),
next_context_number_(1) {}
next_context_number_(1),
pending_enable_async_hook_(false),
pending_disable_async_hook_(false) {}

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

// Enable tracking of async stack traces
if (!enable_async_hook_function_.IsEmpty()) {
Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
if (result.IsEmpty()) {
FatalError(
"node::InspectorAgent::StartIoThread",
"Cannot enable Inspector's AsyncHook, please report this.");
}
}

// Send message to enable debug in workers
Local<Object> process_object = parent_env_->process_object();
Local<Value> emit_fn =
Expand Down Expand Up @@ -537,38 +536,9 @@ void Agent::Stop() {
io_.reset();
enabled_ = false;
}

v8::Isolate* isolate = parent_env_->isolate();
HandleScope handle_scope(isolate);

// Disable tracking of async stack traces
if (!disable_async_hook_function_.IsEmpty()) {
Local<Function> disable_fn = disable_async_hook_function_.Get(isolate);
auto result = disable_fn->Call(parent_env_->context(),
Undefined(parent_env_->isolate()), 0, nullptr);
if (result.IsEmpty()) {
FatalError(
"node::InspectorAgent::Stop",
"Cannot disable Inspector's AsyncHook, please report this.");
}
}
}

void Agent::Connect(InspectorSessionDelegate* delegate) {
if (!enabled_) {
// Enable tracking of async stack traces
v8::Isolate* isolate = parent_env_->isolate();
HandleScope handle_scope(isolate);
auto context = parent_env_->context();
Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
if (result.IsEmpty()) {
FatalError(
"node::InspectorAgent::Connect",
"Cannot enable Inspector's AsyncHook, please report this.");
}
}

enabled_ = true;
client_->connectFrontend(delegate);
}
Expand Down Expand Up @@ -626,6 +596,50 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
v8::Local<v8::Function> disable_function) {
enable_async_hook_function_.Reset(isolate, enable_function);
disable_async_hook_function_.Reset(isolate, disable_function);
if (pending_enable_async_hook_) {
CHECK(!pending_disable_async_hook_);
pending_enable_async_hook_ = false;
EnableAsyncHook();
} else if (pending_disable_async_hook_) {
CHECK(!pending_enable_async_hook_);
pending_disable_async_hook_ = false;
DisableAsyncHook();
}
}

void Agent::EnableAsyncHook() {
if (!enable_async_hook_function_.IsEmpty()) {
Isolate* isolate = parent_env_->isolate();
ToggleAsyncHook(isolate, enable_async_hook_function_.Get(isolate));
} else if (pending_disable_async_hook_) {
CHECK(!pending_enable_async_hook_);
pending_disable_async_hook_ = false;
} else {
pending_enable_async_hook_ = true;
}
}

void Agent::DisableAsyncHook() {
if (!disable_async_hook_function_.IsEmpty()) {
Isolate* isolate = parent_env_->isolate();
ToggleAsyncHook(isolate, disable_async_hook_function_.Get(isolate));
} else if (pending_enable_async_hook_) {
CHECK(!pending_disable_async_hook_);
pending_enable_async_hook_ = false;
} else {
pending_disable_async_hook_ = true;
}
}

void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
HandleScope handle_scope(isolate);
auto context = parent_env_->context();
auto result = fn->Call(context, Undefined(isolate), 0, nullptr);
if (result.IsEmpty()) {
FatalError(
"node::inspector::Agent::ToggleAsyncHook",
"Cannot toggle Inspector's AsyncHook, please report this.");
}
}

void Agent::AsyncTaskScheduled(const StringView& task_name, void* task,
Expand Down
7 changes: 7 additions & 0 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,12 @@ class Agent {
DebugOptions& options() { return debug_options_; }
void ContextCreated(v8::Local<v8::Context> context);

void EnableAsyncHook();
void DisableAsyncHook();

private:
void ToggleAsyncHook(v8::Isolate* isolate, v8::Local<v8::Function> fn);

node::Environment* parent_env_;
std::unique_ptr<NodeInspectorClient> client_;
std::unique_ptr<InspectorIo> io_;
Expand All @@ -102,6 +107,8 @@ class Agent {
DebugOptions debug_options_;
int next_context_number_;

bool pending_enable_async_hook_;
bool pending_disable_async_hook_;
v8::Persistent<v8::Function> enable_async_hook_function_;
v8::Persistent<v8::Function> disable_async_hook_function_;
};
Expand Down
1 change: 1 addition & 0 deletions test/sequential/sequential.status
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ prefix sequential
[true] # This section applies to all platforms

[$system==win32]
test-inspector-async-call-stack : PASS, FLAKY
test-inspector-bindings : PASS, FLAKY
test-inspector-debug-end : PASS, FLAKY
test-inspector-stop-profile-after-done: PASS, FLAKY
Expand Down
79 changes: 79 additions & 0 deletions test/sequential/test-inspector-async-call-stack.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();
common.skipIf32Bits();

const assert = require('assert');
const async_wrap = process.binding('async_wrap');
const { kTotals } = async_wrap.constants;
const inspector = require('inspector');

const setDepth = 'Debugger.setAsyncCallStackDepth';

function verifyAsyncHookDisabled(message) {
assert.strictEqual(async_wrap.async_hook_fields[kTotals], 0);
}

function verifyAsyncHookEnabled(message) {
assert.strictEqual(async_wrap.async_hook_fields[kTotals], 4);
}

// By default inspector async hooks should not have been installed.
verifyAsyncHookDisabled('inspector async hook should be disabled at startup');

const session = new inspector.Session();
verifyAsyncHookDisabled('creating a session should not enable async hooks');

session.connect();
verifyAsyncHookDisabled('connecting a session should not enable async hooks');

session.post('Debugger.enable', () => {
verifyAsyncHookDisabled('enabling debugger should not enable async hooks');

session.post(setDepth, { invalid: 'message' }, () => {
verifyAsyncHookDisabled('invalid message should not enable async hooks');

session.post(setDepth, { maxDepth: 'five' }, () => {
verifyAsyncHookDisabled('invalid maxDepth (string) should not enable ' +
'async hooks');

session.post(setDepth, { maxDepth: NaN }, () => {
verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable ' +
'async hooks');

session.post(setDepth, { maxDepth: 10 }, () => {
verifyAsyncHookEnabled('valid message should enable async hooks');

session.post(setDepth, { maxDepth: 0 }, () => {
verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' +
'async hooks');

runTestSet2(session);
});
});
});
});
});
});

function runTestSet2(session) {
session.post(setDepth, { maxDepth: 32 }, () => {
verifyAsyncHookEnabled('valid message should enable async hooks');

session.post('Debugger.disable', () => {
verifyAsyncHookDisabled('Debugger.disable should disable async hooks');

session.post('Debugger.enable', () => {
verifyAsyncHookDisabled('Enabling debugger should not enable hooks');

session.post(setDepth, { maxDepth: 64 }, () => {
verifyAsyncHookEnabled('valid message should enable async hooks');

session.disconnect();
verifyAsyncHookDisabled('Disconnecting session should disable ' +
'async hooks');
});
});
});
});
}
22 changes: 0 additions & 22 deletions test/sequential/test-inspector-async-hook-teardown-at-debug-end.js

This file was deleted.

0 comments on commit 5886e20

Please sign in to comment.