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

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

Closed
segrey opened this issue Dec 24, 2015 · 14 comments
Closed

Comments

@segrey
Copy link

segrey commented Dec 24, 2015

Currently, Node.js process does not start if the specified debug port is already bound by some another process.
These steps allow to reproduce this behavior (node 5.2.0).

// test.js
console.log('Hello, world');
process.stdin.resume();

Running node --debug=54138 test.js outputs

Debugger listening on port 54138
Hello, world

and does not exit.

Running another node --debug=54138 test.js outputs

Error: listen EADDRINUSE :::54138
    at Object.exports._errnoException (util.js:856:11)
    at exports._exceptionWithHostPort (util.js:879:20)
    at Agent.Server._listen2 (net.js:1238:14)
    at listen (net.js:1274:10)
    at Agent.Server.listen (net.js:1370:5)
    at Object.start (_debug_agent.js:22:9)
    at startup (node.js:72:9)
    at node.js:977:3

and hangs.

It might seem like an expected behavior, unless it interferes with the common pattern of spawning child node processes - child node process command lines are usually constructed using process.execArgv of the main node process. Thus, if the main node process is started in debug mode, child processes will hang unless there is a special handling of process.execArgv that takes care of --debug/--debug-brk.

What do you think if node would try to bind a specified debug port increasing it by one until it succeeds?
Tools can always figure out the really bound debug port by stderr analyzing.

@indutny
Copy link
Member

indutny commented Jan 2, 2016

I think we usually filter those in child_process.fork(). If you are passing process.execArgv explicitly to child process, you should probably filter the desired arguments explicitly as well.

@segrey
Copy link
Author

segrey commented Jan 2, 2016

How about finding an available port if debug port isn't specified explicitly (i.e. --debug or --debug-brk)? Currently, node tries to bind 5858 and fails if this port is already bound.

@indutny
Copy link
Member

indutny commented Jan 3, 2016

What should be a behavior of the parent process? How should it figure out the assigned port?

Also, most of the tools assume that it is running on this port, and such implicit behavior change will break them. Not that it is not something that we can deal with, but just another point in support of current behavior.

@segrey
Copy link
Author

segrey commented Jan 3, 2016

Parent processes (mostly tools like node-inspector/IDEs) could analyze stderr for lines matching /^Debugger listening on port (\d+)$/ to figure out the assigned port.
Yes, tools won't be ready for such a change, but if I understand correctly, the change won't harm either:

  • if initial port is free, child node process will start as before (old behavior is preserved);
  • if initial port is already bound, child node process will start on some another port and tools won't work, but currently child node process fails to start, so tools don't work either in such case. But in their subsequent releases, tools can support child processes debugging.

Probably, there are other ways to allow easy spawning of child node processes, the suggested approach might be not the best way, I just don't see others.

@indutny
Copy link
Member

indutny commented Jan 4, 2016

I think we can probably add a flag for this behavior. It is not absolutely evident to me that it should work like this in generic case, but I totally understand that it will be helpful to you.

@bnoordhuis : what are your thoughts on this?

@sam-github
Copy link
Contributor

The problem is real, but the solution feels fragile. Parsing stderr to figure out where the debug port is horrid, and assumes its even available, which isn't true when node is started standalone and the debugger isn't the parent. If the debugger is the parent, then it makes more sense to reverse the direction of the debug protocol, and have the debugged app connect TO the debugger.

Current behaviour isn't so bad, parent is debuggable, child is not. If you hate that, the other option is to start the parent without --debug, then debug only the specific process you want to, by using signals to turn debugging on.

I'm not sure what other debugging systems do in this case, like gdb. I suspect they load code into the debugged process to trap the fork, and then know what the child process is.

@segrey
Copy link
Author

segrey commented Jan 7, 2016

I think we can probably add a flag for this behavior. It is not absolutely evident to me that it should work like this in generic case

@indutny Could you please clarify which part is not evident to you:

  • starting node process on debug port other than 5858 if port 5858 is already bound (in case when debug port was not specified explicitly, --debug/--debug-brk);
  • or parsing stderr to figure out the real port? If the latter, probably, there could be a better way.

@sam-github Agree, parsing stderr is fragile. It will work only if child process is started with stdio: 'inherit' (to be precise stdio: [any, any, process.stderr] }). Reversing the direction of the debug protocol is a reliable solution, but I guess it is not backward compatible, so tools will need to implement some feature detection (e.g. based on node interpreter version) to enable the new behavior. Also, it will require rework on debugging tools' side. @develar: what do you think?

Current behaviour isn't so bad, parent is debuggable, child is not.

Yes, current behavior isn't so bad, though it isn't good either: child won't even start. Sometimes, it is not clear which process should be debugger - parent or child, so IMO it'd be better to be able to start parent with --debug too. Btw, tools could want to start node processes with --debug-brk to be able to hit breakpoints set in application initialization code, but SIGUSR1 acts as --debug.

@indutny
Copy link
Member

indutny commented Jan 11, 2016

@segrey I was just saying that not every user will want to parser stderr, or want to figure out the actual port on which the process has started.

@develar
Copy link

develar commented Feb 1, 2016

AVA has the same problem — avajs/ava#342 (comment)

I think, it will be really cool if node will do what @segrey suggests (if process was forked). Instead of just throws error EADDRINUSE

On the other hand maybe it will be better to write npm module to overcome. But in this case every author of tool like AVA should be aware about it and do additional work.

@bnoordhuis
Copy link
Member

I opened #5025 to allow --debug-port=0 to bind to a random port. You can then either parse the debuggee's stderr for the port number or retrieve it in-process through the process.debugPort property. Does that help?

@segrey
Copy link
Author

segrey commented Feb 1, 2016

@bnoordhuis IIUC --debug-brk=0 will work too? Then it helps, thank you.
Just curious, how does --debug-port work? Couldn't find any docs about it.

@bnoordhuis
Copy link
Member

It's undocumented, I think. It's for selecting the debug port when the debugger is started later (unlike --debug and --debug-brk, which start it immediately), e.g. with a SIGUSR1 signal.

The main user is the cluster module. It uses it to assign different ports to the cluster's workers.

@segrey
Copy link
Author

segrey commented Feb 2, 2016

Thanks for the explanation and the fix! With this change IDE can run main node.js process with --debug-brk=0 and child processes will safely inherit this cli option. Looks good (well except stderr parsing, but that's a tradeoff).

@segrey segrey closed this as completed Feb 2, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 27, 2017
Allow binding to a randomly assigned port number with `--inspect=0`
or `--inspect-brk=0`.

Refs: nodejs#4419
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 30, 2017
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 <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
jasnell pushed a commit that referenced this issue Jun 5, 2017
Allow binding to a randomly assigned port number with `--inspect=0`
or `--inspect-brk=0`.

PR-URL: #5025
Refs: #4419
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@danielo515
Copy link

from which version is --inspect-brk=0 available ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants