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

Check all local hosts for port availability #56

Merged
merged 12 commits into from Oct 3, 2021

Conversation

mihkeleidast
Copy link
Contributor

@mihkeleidast mihkeleidast commented Aug 7, 2021

Should fix #31 and fix #48.

Note that the solution is copied from #52, with some comments resolved that were not fixed in that PR. Decided to file a new one, because #52 hasn't been updated in months and I would like to get this resolved - lately there have been more and more reports of the "automatic port selection" not working in several projects.

Answering some comments here:

I would have liked to see more comprehensive testing than a single assertion.

Not sure how to test this "more comprehensively". I "resolved" this comment by making the tests run in CI on macos and windows, too, where the added test failed without the fix (see this run). This makes sense to me, because networking and port allocation seem to depend on the operating system. There are however some issues with the 30s test on macos (it sometimes fails), not sure how to fix that?

Are you sure that's a get-port bug and not a Node.js bug? Just want to ensure we don't fix this in the wrong place.

To me it seems like an issue with how get-port checks port availability, and not how Node's net module works. But to be fair I cannot wrap my brain around how networking and port locking works, so :) The fix seems to work, however, at least in tests.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Should we update the docs about this new behavior?

@mihkeleidast
Copy link
Contributor Author

@sindresorhus added a clarification to docs, but i'm not sure if there is anything else that should be documented about this change? I think the new behavior would be the expected behavior.

index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title fix: check all local hosts for port availability Check all local hosts for port availability Oct 3, 2021
@sindresorhus
Copy link
Owner

Tests seems to be failing on macOS.

@mihkeleidast
Copy link
Contributor Author

@sindresorhus yes, I mentioned that in the PR description as well - what would you suggest should be done here? The failing test is not the one added in this PR, rather an older test "ports are locked for up to 30 seconds". That test failed before the changes in this PR, too - I added the multi-os test runs in 74f6443 and the tests failed for that commit already.

I'm not sure what's going on there and I don't have a Mac OS machine to debug that properly.

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