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

debugging and fork (Error: listen EADDRINUSE :::5858) #8495

Closed
pps83 opened this issue Sep 12, 2016 · 6 comments · Fixed by #14140
Closed

debugging and fork (Error: listen EADDRINUSE :::5858) #8495

pps83 opened this issue Sep 12, 2016 · 6 comments · Fixed by #14140
Labels
invalid Issues and PRs that are invalid.

Comments

@pps83
Copy link

pps83 commented Sep 12, 2016

There's lots of random pieces all over the interwebs about that error. Why isn't it properly solved?
IMO if process is being debugged and tries to fork a child then there is no way for it to work if the child also tries to listen on the same debug port. So, why not add special code inside spawn/fork to make sure that debug port isn't inherited?! Seriously, how is that even possible that plain HelloWorld fails in debugger if forked and there is no clue given why it fails. It's easier if something uses default 5858 port, but if you use some other debugger (e.g. webstorm) then it will use some random port and you won't find any clue about why fork fails!

IMO it should make sure that debug port option isn't inherited by forked child to make sure that code works as expected.

@mscdex
Copy link
Contributor

mscdex commented Sep 12, 2016

It should already be doing this.

What node version are you using?

@bnoordhuis
Copy link
Member

I'm going to close this pending more information. I'm inclined to think this is just a rant rather than a good faith bug report if the OP can't be arsed to fill out the issue template.

@bnoordhuis bnoordhuis added the invalid Issues and PRs that are invalid. label Sep 12, 2016
@pps83
Copy link
Author

pps83 commented Sep 12, 2016

@mscdex v6.5.0 on Win10.

Here's sample code:

// test.js
console.log('fork'!=process.argv[2] ? 'parent started' : 'forked started');
if ('fork'!=process.argv[2])
{
    var child = require('child_process').fork(__filename, ['fork'], {silent: false});
    setTimeout(function() { console.log('ending parent process'); }, 2000);
}
else
    setTimeout(function() { console.log('ending forked process'); }, 1000);

if you run it as node test.js then everything is ok, if you run it as node debug test.js and then c to continue under debugger you'll get the error.
Regarding that node should be doing this already. That commit is from August 1, and the 6.5.0 is August 28. Either the fix isn't good, or it didn't make to last release yet.
IMO forked processes should start without debugger, but there should be some fork options like attach_forked_debugger: true so that forked process is automatically attached to current debug session.

@bnoordhuis Sorry, I didn't fill out, it was kind of huge, so I just typed message. I guess you can call this report a rant. I've read about this problem in multiple places (on MS's visual studio node page, on Webstorm) and every time the error is pointed in different direction, or some weird workarounds are offered.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 12, 2016

Note, the link to cluster.js applies to forked workers using the cluster module, not generic fork() calls, which appears to be the problem here.

@pps83
Copy link
Author

pps83 commented Sep 12, 2016

Also, from quick look and my limited js knowledge this isn't a fix, but a workaround for the bug that I'm trying to report here. Looks like they "patch" arguments to be passed to fork, while fork/spawn themselves have to be properly fixed internally. Similarly, you can "patch" my test.js code by changing fork options from {silent: false} to {silent: false, execArgv: []} so that forked child doesn't try to listen on the same debug port. In my case I simply needed to start forked process and I don't need to debug them, it was total WTF to figure out why it just didn't work at all.

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 13, 2016

Colin is right that cluster.fork() fixes up execArgv before spawning a worker, child_process.fork() does not.

We could move the fix-up logic to child_process.fork() but that might end up breaking more than it solves. child_process.fork() is designed to be configurable, cluster.fork() is not; implicitly rewriting arguments might interfere with the user's intentions.

#5025 would alleviate this issue to some extent. You pick a random port by binding to port zero and inspect process.debugPort afterwards (by printing it or sending it to the master or whatever.)

You can pick random port numbers now, of course, but it's not as race-free as having the operating system pick a port number for you.

refack pushed a commit to mutantcornholio/node that referenced this issue Jul 14, 2017
* Capitalization and punctuation.

* `setupMaster` contained info about `settings` which where incomplete.

PR-URL: nodejs#14140
Fixes: nodejs#8495
Fixes: nodejs#12941
Refs: nodejs#9659
Refs: nodejs#13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
refack pushed a commit to mutantcornholio/node that referenced this issue Jul 14, 2017
10 ports for each test-case is too much.
Not enough ports for new test cases, considering ~100 ports per file.

PR-URL: nodejs#14140
Fixes: nodejs#8495
Fixes: nodejs#12941
Refs: nodejs#9659
Refs: nodejs#13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
refack pushed a commit to mutantcornholio/node that referenced this issue Jul 14, 2017
Added an option to override inspector port for workers using
`settings.inspectPort` will override default port incrementing behavior.
Also, using this option allows to set 0 port for the whole cluster.

PR-URL: nodejs#14140
Fixes: nodejs#8495
Fixes: nodejs#12941
Refs: nodejs#9659
Refs: nodejs#13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this issue Jul 18, 2017
* Capitalization and punctuation.

* `setupMaster` contained info about `settings` which where incomplete.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this issue Jul 18, 2017
10 ports for each test-case is too much.
Not enough ports for new test cases, considering ~100 ports per file.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Added an option to override inspector port for workers using
`settings.inspectPort` will override default port incrementing behavior.
Also, using this option allows to set 0 port for the whole cluster.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
* Capitalization and punctuation.

* `setupMaster` contained info about `settings` which where incomplete.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
10 ports for each test-case is too much.
Not enough ports for new test cases, considering ~100 ports per file.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Added an option to override inspector port for workers using
`settings.inspectPort` will override default port incrementing behavior.
Also, using this option allows to set 0 port for the whole cluster.

PR-URL: #14140
Fixes: #8495
Fixes: #12941
Refs: #9659
Refs: #13761
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@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
invalid Issues and PRs that are invalid.
Projects
None yet
4 participants