inspector: Do not crash if the port is n/a #7874
Conversation
nodejs-github-bot added c++ 
src/inspector_agent.cc Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| LGTM |
Node process will no longer terminate with an assertion if the inspector port is not available.
| I added a missing space, as required by linter. |
| LGTM |
| Landed as 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>
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
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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