debugger: bind to random port with --debug-port=0 by bnoordhuis · Pull Request #5025 · nodejs/node

bnoordhuis · 2016-02-01T13:14:18Z

Allow binding to a randomly assigned port number with --debug-port=0.

Refs: #4419

R=@indutny

ChALkeR · 2016-02-01T13:19:09Z

This looks like a semver-minor to me.

thefourtheye · 2016-02-01T13:35:15Z

src/node.cc

Why this is necessary?

It's the opposite of necessary, the return statement isn't necessary. :-)

Duh :D In my defense I was Reviewing in mobile.

develar · 2016-02-01T13:57:03Z

I think, new CLI parameter is not required. Old --debug and --debug-brk could be modified to use 0 as any free port.

Also, debuggers use --debug-brk (run and stop on first line — wait debugger connection). --debug used only to debug already running nodejs process.

update: it seems, issue title is not correct — new logic is applied to all 3 flags and not bound only to --debug-port.

develar · 2016-02-01T14:05:45Z

We (JetBrains) like this solution.

#4419 is too magic. Node is an engine. Strictly speaking, it should be handled on an application level.

Node feature --debug-brk=0 (this issue) means that JetBrains debugger (or node-inspector) must be modified also — IDE should run main node process with --debug-brk=0 (forked child will be also run with this parameter (default nodejs behaviour)).

But as result, tools like AVA will be debuggable without any efforts from tool author.

bnoordhuis · 2016-02-01T14:46:45Z

I think, new CLI parameter is not required.

It's not new, it's an existing command line switch.

update: it seems, issue title is not correct — new logic is applied to all 3 flags and not bound only to --debug-port.

Correct, but that doesn't fit in 50 columns. :-)

thefourtheye · 2016-02-01T14:52:45Z

LGTM

jasnell · 2016-02-01T17:34:09Z

LGTM

bnoordhuis · 2016-02-01T23:39:36Z

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

evanlucas · 2016-02-01T23:41:48Z

lib/_debug_agent.js

so could it be possible that an error was passed to the callback from listen and this.address().port is undefined? If so, that would cause an abort in debug-agent right? Should we guard for that?

net.Server#listen() doesn't work that way, it emits an 'error' event when it can't bind to a port.

Ah good point. I was incorrectly assuming that the connection listener would still be called, just with an error. Carry on

evanlucas · 2016-02-02T00:04:15Z

LGTM

bnoordhuis · 2016-02-02T13:32:20Z

New CI attempt: https://ci.nodejs.org/job/node-test-pull-request/1504/ - previous one was hit by a network outage, it looks like.

cjihrig · 2016-02-02T14:14:45Z

LGTM

bnoordhuis · 2016-02-02T16:01:39Z

Grr, more RemotingSystemExceptions and IOExceptions. One more try: https://ci.nodejs.org/job/node-test-pull-request/1506/

bnoordhuis · 2016-02-02T23:20:18Z

A seemingly genuine failure on one of the Windows buildbots:

not ok 42 test-debug-port-zero.js
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: 3221226505 === 0
#     at ChildProcess.proc.on.common.mustCall (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win10\test\parallel\test-debug-port-zero.js:10:10)
#     at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win10\test\common.js:395:15)
#     at emitTwo (events.js:101:13)
#     at ChildProcess.emit (events.js:186:7)
#     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

https://ci.nodejs.org/job/node-test-binary-windows/922/RUN_SUBSET=1,VS_VERSION=vs2015,label=win10/console

Exit code 3221226505 is C0000409 is "Stack buffer overflow."

bnoordhuis · 2016-02-04T17:13:02Z

I suspect the failure is unrelated and only exposed, not caused, by this change. Added another test to validate that. CI: https://ci.nodejs.org/job/node-test-pull-request/1543/

EDIT: To be clear, the second commit is not supposed to land as-is.

bnoordhuis · 2016-02-04T18:36:22Z

Interestingly, the new test passes on all platforms. The old test passes too except on win10+vs2015; win2012r2+vs2015 and win20082r2+vs2013 are fine. I'm inclined to scrap the previous test and go with the new one.

3y3 · 2016-02-05T17:03:50Z

How we can receive information about debug port of each child from cluster master?
How we can start all cluster on random ports?

bnoordhuis · 2016-02-05T19:16:15Z

Can I get one more round of review? I worked over the test and added two more. The gist of the PR hasn't changed but I had to add some machinery to make testing reliable.

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

bnoordhuis · 2016-02-05T19:18:04Z

@3y3 Take a look at the cluster test I just added. If you start the master with --debug=0 or --debug-port=0, it gets propagated to the workers. If you want to know the port a worker is listening on, either parse its stderr or ask it for its process.debugPort property (that's what the test does.)

3y3 · 2016-02-05T20:13:04Z

test/parallel/test-debug-port-zero-cluster.js

Maybe port > 1024?

Yes, that would be more correct.

jasnell · 2016-03-22T00:26:21Z

@bnoordhuis ... what's the status on this?

bnoordhuis

Updated, @sam-github PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/8332/

Question: should the call to env->inspector_agent()->Start() call debug_options.set_port()?

sam-github · 2017-05-27T15:32:05Z

I think the options should mirror what was set on the CLI (or the default), IMO., anything else is more confusing. When the listener is stopped and started the port will change.

bnoordhuis · 2017-05-28T10:04:16Z

@sam-github Can you (re)review? It's not clear to me if you were asking me to change anything.

sam-github · 2017-05-29T00:33:32Z

Ben:

Question: should the call to env->inspector_agent()->Start() call debug_options.set_port()?

Sam:
Answer: I think the options should mirror what was set on the CLI (or the default), IMO., anything else is more confusing. When the listener is stopped and started the port will change. So --> No, to not call set_port() with the actual ephemeral port

cjihrig

Still LGTM

Allow binding to a randomly assigned port number with `--inspect=0` or `--inspect-brk=0`. PR-URL: nodejs#5025 Refs: nodejs#4419 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

bjrmatos · 2017-05-31T16:56:57Z

sorry to comment on a closed issue, just one question, is this change present in latest 8.0? i'm trying to use it but it fails with Debug port must be in range 1024 to 65535.

bnoordhuis · 2017-05-31T17:02:11Z

I just replied to nodejs/help#649 but to answer your question, I expect it will be released in v8.1.0.

bjrmatos · 2017-05-31T17:02:56Z

thnks!

Allow binding to a randomly assigned port number with `--inspect=0` or `--inspect-brk=0`. PR-URL: #5025 Refs: #4419 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](nodejs@135f4e6643)] [nodejs#13367](nodejs#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](nodejs@968596ec77)] [nodejs#13306](nodejs#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](nodejs@ffa7debd7a)] [nodejs#13487](nodejs#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](nodejs@6e0eccd7a1)] [nodejs#13316](nodejs#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](nodejs@c756efb25a)] [nodejs#13173](nodejs#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](nodejs@cc6ec2fb27)] [nodejs#5025](nodejs#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](nodejs@6aeb555cc4)] [nodejs#13374](nodejs#13374).

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](135f4e6643)] [#13367](#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](968596ec77)] [#13306](#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](ffa7debd7a)] [#13487](#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](6e0eccd7a1)] [#13316](#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](c756efb25a)] [#13173](#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](cc6ec2fb27)] [#5025](#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](6aeb555cc4)] [#13374](#13374).

gibfahn · 2018-01-15T21:41:07Z

Release team decided not to backport to v6.x, as the inspector is substantially different in that version, let us know if you think it should be backported.

james-rabbit · 2018-07-03T22:07:11Z

Is this feature documented somewhere? I wasn't able to find much information about it.
Is there any chance of multiple forked children trying to use the same port for debugging?

bnoordhuis added the debugger label Feb 1, 2016

bnoordhuis mentioned this pull request Feb 1, 2016

Bind an available port for debugging if the specified debug port is already bound #4419

Closed

thefourtheye reviewed Feb 1, 2016
View reviewed changes

bnoordhuis added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2016

evanlucas reviewed Feb 1, 2016
View reviewed changes

bnoordhuis force-pushed the fix4419 branch from 09ebf94 to ce19f43 Compare February 4, 2016 17:11

bnoordhuis mentioned this pull request Feb 5, 2016

Listen on unix socket in debugger #5059

Closed

bnoordhuis force-pushed the fix4419 branch from ce19f43 to fa331a5 Compare February 5, 2016 19:13

3y3 reviewed Feb 5, 2016
View reviewed changes

develar mentioned this pull request Mar 7, 2016

fix #342 set child process debug port to available avajs/ava#613

Closed

bnoordhuis force-pushed the fix4419 branch from fa331a5 to b4e5640 Compare April 5, 2016 15:36

bnoordhuis force-pushed the fix4419 branch from fe4cd00 to 59e9748 Compare May 27, 2017 12:04

bnoordhuis commented May 27, 2017

View reviewed changes

sam-github approved these changes May 29, 2017

View reviewed changes

refack approved these changes May 29, 2017

View reviewed changes

cjihrig approved these changes May 29, 2017

View reviewed changes

bnoordhuis added inspector Issues and PRs related to the V8 inspector protocol and removed stalled Issues and PRs that are stalled. debugger labels May 30, 2017

bnoordhuis closed this May 30, 2017

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

conartist6 mentioned this pull request Oct 25, 2017

Impossible to debug multi-threaded processes on headless server. #16469

Closed

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

Dzenly mentioned this pull request Jun 20, 2018

Support for --inspect-brk=0 for node.js debugging. microsoft/vscode#52373

Closed

This was referenced Apr 4, 2019

Increment debug port for each child process. tj/commander.js#874

Closed

error Error: listen EADDRINUSE :::5858 when using node debug tj/commander.js#533

Closed

Cannot use inspect in git-style mode tj/commander.js#838

Closed

Uh oh!

Conversation

bnoordhuis commented Feb 1, 2016

Uh oh!

ChALkeR commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

develar commented Feb 1, 2016

Uh oh!

develar commented Feb 1, 2016

Uh oh!

bnoordhuis commented Feb 1, 2016

Uh oh!

thefourtheye commented Feb 1, 2016

Uh oh!

jasnell commented Feb 1, 2016

Uh oh!

bnoordhuis commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Feb 2, 2016

Uh oh!

bnoordhuis commented Feb 2, 2016

Uh oh!

cjihrig commented Feb 2, 2016

Uh oh!

bnoordhuis commented Feb 2, 2016

Uh oh!

bnoordhuis commented Feb 2, 2016

Uh oh!

bnoordhuis commented Feb 4, 2016

Uh oh!

bnoordhuis commented Feb 4, 2016

Uh oh!

3y3 commented Feb 5, 2016

Uh oh!

bnoordhuis commented Feb 5, 2016

Uh oh!

bnoordhuis commented Feb 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented May 27, 2017

Uh oh!

bnoordhuis commented May 28, 2017

Uh oh!

sam-github commented May 29, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

bjrmatos commented May 31, 2017

Uh oh!

bnoordhuis commented May 31, 2017

Uh oh!

bjrmatos commented May 31, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

james-rabbit commented Jul 3, 2018

Uh oh!

Reviewers

Assignees