Skip to content
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.

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

inspector: libuv notification on incoming message #11617

Merged
merged 0 commits into from
Mar 3, 2017
Merged

inspector: libuv notification on incoming message #11617

merged 0 commits into from
Mar 3, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Feb 28, 2017

Currently Inspector posts a V8 "task" when a message is incoming. To
make sure messages are processed even when no JS is executed (e.g. while
waiting for I/O or timer), inspector will now post a libuv request.

Fixes: #11589

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x labels Feb 28, 2017
@kfarnung
Copy link
Contributor

The code change looks good. I tested it out on Windows using VSCode and I no longer get the request timeout errors in the console. The breakpoint gets set immediately.

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Feb 28, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with two nits.

@@ -606,6 +608,7 @@ void AgentImpl::PostIncomingMessage(InspectorAction action, int session_id,
platform_->CallOnForegroundThread(isolate,
new DispatchOnInspectorBackendTask(this));
isolate->RequestInterrupt(InterruptCallback, this);
uv_async_send(&main_thread_req_);
Copy link
Member

Choose a reason for hiding this comment

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

Can you CHECK_EQ(0, uv_async_send(&main_thread_req_)) here?

};

exports.startNodeForInspectorTest = function(callback, opt_skip_break,
opt_script_contents) {
Copy link
Member

Choose a reason for hiding this comment

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

A default param might be nicer for the second argument, i.e., function(callback, arg = '--inspect-brk', contents). The string '--inspect' makes the intent clearer at the call site than a boolean does.

Style issue: we generally use camelCase for arguments and locals in JS land.

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 3, 2017

@bnoordhuis Thank you for the review. I addressed the comments.

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 3, 2017

@eugeneo eugeneo closed this Mar 3, 2017
@eugeneo eugeneo deleted the libuv_ping branch March 3, 2017 19:41
@eugeneo eugeneo merged commit f01fd2a into nodejs:master Mar 3, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 3, 2017

Landed as f01fd2a

addaleax pushed a commit that referenced this pull request Mar 5, 2017
Currently Inspector posts a V8 "task" when a message is incoming. To
make sure messages are processed even when no JS is executed (e.g. while
waiting for I/O or timer), inspector will now post a libuv request.

Fixes: #11589
PR-URL: #11617
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
@MylesBorins
Copy link
Contributor

I would love to see an effort for a wholesale update of the inspect stuff on v6. If not possible please lmk

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 17, 2017

I don't think v6.x has this issue, it was introduced in the refactoring that was never backported...

Is there a list of inspector issues in v6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breakpoints can't be set by a debugger (Chrome or VS Code) while the Node program is running
6 participants