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.

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

Jump to bottom

inspector: Do not crash if the port is n/a #7874

Closed
wants to merge 1 commit into from
Closed

inspector: Do not crash if the port is n/a #7874

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Jul 25, 2016 •
edited by ofrobots
Loading

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

deps:v8_inspector

Description of change

Node process will no longer terminate with an assertion if the inspector
port is not available.

CC: @ofrobots

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jul 25, 2016
bnoordhuis
bnoordhuis reviewed Jul 26, 2016
View reviewed changes
src/inspector_agent.cc
@@ -207,6 +207,7 @@ class AgentImpl {
bool wait_;
bool connected_;
bool shutting_down_;
bool startup_failed_;
Copy link
Member

Choose a reason for hiding this comment

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

This is starting to look like a state machine spread across several booleans. Consolidating them into an enum is cleaner and easier to reason about, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. I introduced initial set of states. Note that I am still working a refactoring that will split "inspector agent" (the one that lives on the main thread and talks to V8) and "inspector server" (that talks to a client over the socket - including all HTTP responses and such). Then there will be 2 objects with likely two states. In the current implementation, adding states for all flags (like shutting_down_ and wait_) would make the variable accessed (and modified) from both threads - which might introduce some race conditions (or require more synchronization than is neccessary now).

@bnoordhuis
Copy link
Member

LGTM

@ofrobots
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/3443/

@eugeneo
inspector: Do not crash if the port is n/a
d13c544 
Node process will no longer terminate with an assertion if the inspector
port is not available.
@eugeneo
Copy link
Contributor Author

eugeneo commented Jul 29, 2016

I added a missing space, as required by linter.

@jasnell
Copy link
Member

jasnell commented Jul 29, 2016

LGTM

@ofrobots
Copy link
Contributor

New CI: https://ci.nodejs.org/job/node-test-pull-request/3454/

@ofrobots
Copy link
Contributor

Landed as f789eb3.

@ofrobots ofrobots closed this Jul 29, 2016
ofrobots pushed a commit that referenced this pull request Jul 29, 2016
@ofrobots
inspector: Do not crash if the port is n/a
f789eb3 
Node process will no longer terminate with an assertion if the
inspector port is not available.

PR-URL: #7874
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
@eugeneo eugeneo deleted the stop_when_no_port branch August 2, 2016 23:37
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@cjihrig
inspector: Do not crash if the port is n/a
b75ca50 
Node process will no longer terminate with an assertion if the
inspector port is not available.

PR-URL: #7874
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Conflicts:
	src/node.cc
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eugeneo @bnoordhuis @ofrobots @jasnell @MylesBorins @nodejs-github-bot