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

src: allow inspector to be run on ephemeral port #12093

Closed

Conversation

sam-github
Copy link
Contributor

Its impossible to know in general whether any specific port will be
available or not, so its quite useful to run the inspector on an
ephemeral port, chosen by the operating system from its free ports.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger labels Mar 28, 2017
This PR allows `node --inspect=0` or `node --inspect=192.168.3.1:0`.

Its impossible to know in general whether any specific port will be
available or not, so its quite useful to run the inspector on an
ephemeral port, chosen by the operating system from its free ports.
@sam-github sam-github force-pushed the allow-ephemeral-inspect-ports branch from 3eca777 to c4a465a Compare March 28, 2017 15:24
@eugeneo
Copy link
Contributor

eugeneo commented Mar 28, 2017

This will also change behavior of the debugger (e.g. --debug flag). Is this expected?

@sam-github
Copy link
Contributor Author

Interesting, the --debug option is completely undocumented, other than it being mentioned that it exists in the online docs (its not mentioned in the --help output for any node from 0.10 to 7.x).

But I would expect 0 to be valid there, and I just tried, and it works fine.

@eugeneo
Copy link
Contributor

eugeneo commented Mar 28, 2017

I think it would be nice to switch inspector tests to use :0 port and to have a dedicated test case to test default or explicitly specified port - this way it will be easier to run tests in parallel.

Also, CI test failures seem to be caused by this PR.

@gibfahn
Copy link
Member

gibfahn commented Mar 28, 2017

have a dedicated test case to test default or explicitly specified port

Default --inspect port test: deps/node-inspect/test/node-inspect.test.js

There is currently only 1 --inspect test in sequential/: test/sequential/test-debugger-debug-brk.js

@sam-github
Copy link
Contributor Author

I'm still working through the tests, I've a slow machine.

I agree that this feature may allow a number of the sequential tests to be run in parallel, but that should be done in a follow up, not here.

@richardlau
Copy link
Member

cc @nodejs/diagnostics

@bnoordhuis
Copy link
Member

Way ahead of you, Sam: #5025 :-) Unfortunately, the CI is as red there as it is here.

@eugeneo
Copy link
Contributor

eugeneo commented Mar 28, 2017 via email

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Mar 28, 2017
@sam-github sam-github closed this Apr 3, 2017
@sam-github sam-github deleted the allow-ephemeral-inspect-ports branch April 17, 2017 20:39
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.

7 participants