Skip to content

inspector: track async stacks when necessary #16260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ofrobots wants to merge 3 commits into from
Closed

inspector: track async stacks when necessary #16260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50c0bc1
inspector: track async stacks when necessary
ofrobots Oct 17, 2017
df628ce
[SQUASH] address comments
ofrobots Oct 18, 2017
0a48cf7
[SQUASH] fix typos in test
ofrobots Oct 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[SQUASH] address comments
  • Loading branch information
@ofrobots
ofrobots committed Oct 18, 2017
commit df628ce5b9234e250c8bb9eab3278d2384922697
95 changes: 33 additions & 62 deletions src/inspector_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,14 +535,7 @@ void Agent::Stop() {

// 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.");
}
ToggleAsyncHook(isolate, disable_async_hook_function_.Get(isolate));
}
}

Expand Down Expand Up @@ -607,34 +600,21 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
disable_async_hook_function_.Reset(isolate, disable_function);
if (pending_enable_async_hook_) {
pending_enable_async_hook_ = false;
EnableAsyncHook(isolate);
ToggleAsyncHook(isolate, enable_async_hook_function_.Get(isolate));
} else if (pending_disable_async_hook_) {
pending_disable_async_hook_ = false;
DisableAsyncHook(isolate);
ToggleAsyncHook(isolate, disable_async_hook_function_.Get(isolate));
}
}

void Agent::EnableAsyncHook(Isolate* isolate) {
CHECK(!enable_async_hook_function_.IsEmpty());
Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
auto context = parent_env_->context();
auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
if (result.IsEmpty()) {
FatalError(
"node::inspector::Agent::EnableAsyncHook",
"Cannot enable Inspector's AsyncHook, please report this.");
}
}

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

@bnoordhuis bnoordhuis Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods could be dedup'd, e.g., by passing the Persistent<Function> to use as a parameter (and the name in the error message as a string.)

Copy link
Copy Markdown
Contributor Author

@ofrobots ofrobots Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

Expand Down Expand Up @@ -664,10 +644,15 @@ void Agent::InterceptAsyncStackDepthMessage(const StringView& message) {
// --inspect-brk, the debugger must necessarily attach before much JavaScript
// can execute. The Debugger.setAsyncCallStackDepth message arrives too early
// and we must intercept this in C++.
//
// TODO: for performance reasons, it would be good to pre-scan the string for
// 'Debugger.setAsyncCallStackDepth' and return early if not found.

v8::Isolate* isolate = parent_env_->isolate();
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method looks like it needs a HandleScope.

Copy link
Copy Markdown
Contributor Author

@ofrobots ofrobots Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

HandleScope handle_scope(isolate);
Local<Context> context = parent_env_->context();

v8::TryCatch try_catch(isolate); // catch and ignore exceptions.
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/catch/Catch/ and two spaces before the //. (Although I don't think the comment adds much.)

MaybeLocal<String> string =
String::NewFromTwoByte(isolate, message.characters16(),
Copy link
Copy Markdown
Contributor

@eugeneo eugeneo Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringView does not convert between encodings. I believe, right now all messages are 16 bit. Can you add CHECK (!message.is8Bit()) just to be sure?

v8::NewStringType::kNormal, message.length());
Expand All @@ -688,28 +673,23 @@ void Agent::InterceptAsyncStackDepthMessage(const StringView& message) {
// We ignore (return early) on malformed messages and let v8-inspector deal
// with them.

MaybeLocal<Value> maybe_parsed =
v8::JSON::Parse(context, string.ToLocalChecked());
if (maybe_parsed.IsEmpty()) {
return;
}

Local<Value> parsed = maybe_parsed.ToLocalChecked();
if (!parsed->IsObject()) {
Local<Value> scratch;
Local<Object> parsed;
if (!v8::JSON::Parse(context, string.ToLocalChecked()).ToLocal(&scratch) ||
!scratch->IsObject() ||
!scratch->ToObject(context).ToLocal(&parsed)) {
return;
}
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could shorten this to:

Local<Value> scratch;
Local<Object> parsed;
if (!v8::JSON::Parse(context, string).ToLocal(&scratch) ||
    !scratch->IsObject() ||
    !scratch->ToObject(context).ToLocal(&parsed)) {
  return;
}

(General observation, applies to not just this block of code.)

Copy link
Copy Markdown
Contributor Author

@ofrobots ofrobots Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Done.

It might be possible to condense the code a bit further along these lines, but I left it at a point which seemed to be a good balance between terseness and readability.


Local<Object> object = parsed.As<Object>();

Local<Value> method;
if (!object->Get(context, parent_env_->method_string()).ToLocal(&method) ||
if (!parsed->Get(context, parent_env_->method_string()).ToLocal(&method) ||
!method->IsString() ||
!method->StrictEquals(parent_env_->async_stack_depth_string())) {
return;
}

Local<Value> params;
if (!object->Get(context, parent_env_->params_string()).ToLocal(&params) ||
if (!parsed->Get(context, parent_env_->params_string()).ToLocal(&params) ||
!params->IsObject()) {
return;
}
Expand All @@ -721,35 +701,26 @@ void Agent::InterceptAsyncStackDepthMessage(const StringView& message) {
return;
}

Maybe<double> maybe_depth = depth_value->NumberValue(context);
if (maybe_depth.IsNothing()) {
return;
}

double depth = maybe_depth.FromJust();
double depth = depth_value->NumberValue(context).FromJust();
if (depth == 0) {
// Disable.
if (!disable_async_hook_function_.IsEmpty()) {
DisableAsyncHook(isolate);
if (pending_enable_async_hook_) {
CHECK(!pending_disable_async_hook_);
pending_enable_async_hook_ = false;
} else if (!disable_async_hook_function_.IsEmpty()) {
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;
}
pending_disable_async_hook_ = true;
}
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be flattened like this?

if (pending_enable_async_hook_) {
  // ...
} else if (!disable_async_hook_function_.IsEmpty()) {
  // ...
} else {
  // ...
}

If not, a code comment explaining why is probably in order.

Copy link
Copy Markdown
Contributor Author

@ofrobots ofrobots Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} else {
// Enable.
if (!enable_async_hook_function_.IsEmpty()) {
EnableAsyncHook(isolate);
if (pending_disable_async_hook_) {
CHECK(!pending_enable_async_hook_);
pending_disable_async_hook_ = false;
} else if (!enable_async_hook_function_.IsEmpty()) {
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;
}
pending_enable_async_hook_ = true;
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/inspector_agent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ class Agent {
void ContextCreated(v8::Local<v8::Context> context);

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

node::Environment* parent_env_;
std::unique_ptr<NodeInspectorClient> client_;
Expand Down