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: no crash when WS server can't start #10878

Merged
merged 1 commit into from
Jan 20, 2017
Merged

inspector: no crash when WS server can't start #10878

merged 1 commit into from
Jan 20, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Jan 18, 2017

This change also changes error message to make it consistent with the
one printed by the debugger.

Fixes: #10858

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: now it uses host name from the debug options. Messages were slightly altered.

@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 inspector Issues and PRs related to the V8 inspector protocol labels Jan 18, 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. This doesn't change the default behavior of binding to 127.0.0.1, does it?

@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 18, 2017

@bnoordhuis - it will bind to 127.0.0.1 as per https://github.com/nodejs/node/blob/master/src/node_debug_options.cc#L62 - the fact that host was hardcoded was an oversight.

@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 19, 2017

CI is green - https://ci.nodejs.org/job/node-test-pull-request/5939/ - even though the GitHub integration shows failure on ARM...

This change also changes error message to make it consistent with the
one printed by the debugger.

Fixes: #10858
PR-URL: #10878
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 20, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/5957/ (all green, despite what GitHub shows)

@eugeneo eugeneo closed this Jan 20, 2017
@eugeneo eugeneo merged commit ba776b3 into nodejs:master Jan 20, 2017
@eugeneo eugeneo deleted the no-assert branch January 20, 2017 17:13
targos pushed a commit that referenced this pull request Jan 28, 2017
This change also changes error message to make it consistent with the
one printed by the debugger.

Fixes: #10858
PR-URL: #10878
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
This change also changes error message to make it consistent with the
one printed by the debugger.

Fixes: nodejs#10858
PR-URL: nodejs#10878
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
This change also changes error message to make it consistent with the
one printed by the debugger.

Fixes: nodejs#10858
PR-URL: nodejs#10878
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

A backport PR is required in order for this to land on 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.

inspector aborts when another process is running the inspector
7 participants