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

Fixes issues where Node doesn't error if port is in use. #207

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bacrossland
Copy link

This fixes #206

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I was just reading your issue about this. I guess a refactor at some point broke it. So it doesn't happen again, can you add a test for both of these things?

For example, there should be at least one test that doesn't pass if I put the .listen line back above the .on calls and at lest one test that does not pass if I remove the 0.0.0.0 argument.

As a side note, won't the 0.0.0.0 change be a breaking semver major change? Looking at the Node.js docs without that it will listen on both IPv4 and IPv6 but with the 0.0.0.0 it will only listen on IPv4, right? If so, is there a way to keep it listening on both?

@bacrossland
Copy link
Author

bacrossland commented Jul 19, 2018

Working on a solution now. I'm going to update this PR to better describe what is happening and how the fix would solve that.

@bacrossland
Copy link
Author

bacrossland commented Jul 24, 2018

After a lot of research, I have reverted part of my fix and provided a test for it. The issue that I was having is an OS specific issue. If you have IPv6 enabled it will not necessarily block IPv4 for listening. I've added a comment block to explain more what is happening incase other engineers have this issue with their app.

I've also added in a catch for an error condition that may return on Linux when kernel option ipv6.disable=1 is used. I did not force listening on IPv4 under that condition and instead opted for alerting the engineer of the issue so they can make a decision based on their environment.

Sources:

ipv6only listen/bind option
nodejs/node#17664

Why listening "::" (all IPv6) also get "0.0.0.0"(all IPv4) listened?
nodejs/node#9390

IPv6: Is ::' equivalent to 0.0.0.0' when listening for connections?
https://stackoverflow.com/questions/27480094/ipv6-is-equivalent-to-0-0-0-0-when-listening-for-connections

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! Unfortunately the test is not going to cut it. It's just too fragile and asserts nothing about the program's behavior, only that a specific piece of source code exists. All the tests in this project are invoking through the bin file to start apps, so all the work is there to start the app and capture output. Just needs to be done for this test so it's not coupled to specific source code styling.

I can help out on this in a few weeks if you're not able to get to it as well

@bacrossland
Copy link
Author

Sorry I haven't gotten back to this. I will try to get to this before the end of the month.

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

Successfully merging this pull request may close these issues.

Server listens on port already in use
2 participants