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 aborts when another process is running the inspector #10858

Closed
trevnorris opened this issue Jan 17, 2017 · 3 comments · Fixed by #10878
Closed

inspector aborts when another process is running the inspector #10858

trevnorris opened this issue Jan 17, 2017 · 3 comments · Fixed by #10878
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.

Comments

@trevnorris
Copy link
Contributor

  • Version: 8.0.0-pre
  • Platform: Linux
  • Subsystem: inspector

Description:
Running node --inspect in the background causes make cctest to abort. The same thing for trying to run node --inspect twice.

Reproduction:
Run node --inspect in one window, and make cctest in another window. The test should fail with:

../test/cctest/test_inspector_socket_server.cc:381: Failure
Value of: server->Start(&loop)
  Actual: false
Expected: true
[--I] signal   0x6caf08
[-AI] async    0x6cad50
[---] <unknown> 0x7fff84cd1998
Makefile:117: recipe for target 'cctest' failed
make: *** [cctest] Segmentation fault (core dumped)

Alternative is to run node --inspect twice. Which gives me:

trevnorris:node (async-wrap-eps-impl) $ ./node --inspect
Unable to open devtools socket: address already in use
Unable to open devtools socket: Unknown system error 0
> /var/projects/node/out/Release/node[17258]: ../src/node.cc:3739:void node::EnableDebug(node::Environment *): Assertion `debugger_running' failed.
 1: node::Abort() [./node]
 2: 0x10abe44 [./node]
 3: 0x10b547b [./node]
 4: node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) [./node]
 5: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [./node]
 6: node::Start(int, char**) [./node]
 7: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
 8: _start [./node]
Aborted (core dumped)

/cc @nodejs/v8-inspector

@trevnorris trevnorris added test Issues and PRs related to the tests. inspector Issues and PRs related to the V8 inspector protocol labels Jan 17, 2017
@eugeneo
Copy link
Contributor

eugeneo commented Jan 18, 2017

#10861 changes the test so it uses autodiscovered port (so it does not conflict with another instance of the test running or with node --inspect).

Not sure what is the desired behavior for running two instances of node with --inspect...

@joyeecheung
Copy link
Member

joyeecheung commented Jan 18, 2017

IMO this should not abort, instead just exit normally (EDIT: I think --debug just throws an exception, doesn't exit, anyway if it does exit, should exit with a non-zero exit code) throw an exception with some message telling the address/socket is being used(unless people pass in --abort-on-uncaught-exceptions).

--debug uses Error: listen EADDRINUSE 127.0.0.1:5858, --inspect currently shows Unable to open devtools socket: address already in use but I would say mimicking the behavior of --debug is less confusing(more familiar) to users. And I think an additional hint like "Are you running another node instance with --inspect?" would be more friendly to beginners(googling for EADDRINUSE 5858 shows a lot of confused questions).

@eugeneo
Copy link
Contributor

eugeneo commented Jan 18, 2017

Assert was a bug, I created a pull request with a fix. That pull request also changes the message to be more in line with what the old debugger prints.

As to "Are you running another node instance with --inspect?" - the error message needs to be generic as the code does not know if the failure is because the port is bound or if there's some other failure.

italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 25, 2017
This ensures that cctest can be ran concurrently with other instances of
cctest or while the node is ran with --inspect.

Ref: nodejs#10858
PR-URL: nodejs#10861
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 issue Jan 27, 2017
This ensures that cctest can be ran concurrently with other instances of
cctest or while the node is ran with --inspect.

Ref: nodejs#10858
PR-URL: nodejs#10861
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue 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 pushed a commit to italoacasas/node that referenced this issue 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 issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants