-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix: already in use port #10638 #10651
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
But I'm afraid this will change the behavior when
- DNS resolves
localhost
to::1
- and either
- using Node.js 17+ and not setting
dns.setDefaultResultOrder('ipv4first')
- using Node.js 17< and setting
dns.setDefaultResultOrder('verbatim')
- using Node.js 17+ and not setting
. In that situation, Vite will now listen to 127.0.0.1
instead of ::1
. This is a breaking change.
Also I don't think behaving differently from Node.js's default is a good thing. And this is the problem we had in Vite 2.
I think we can just check whether 0.0.0.0:port
is occupied rather than changing the default host to listen.
77bc580
to
2d98286
Compare
2d98286
to
0429393
Compare
Agreed, if I get it right, the last commit is a proposal I'm not very proud of but it works :/ What do you think ? |
(thanks for doing this work!) I'm worried actually opening on 0.0.0.0 might trip firewalls, if an OS were set up to allow listening on localhost but not external ports. But I haven't seen this. |
@@ -178,8 +178,19 @@ export async function httpServerStart( | |||
httpServer.on('error', onError) | |||
|
|||
httpServer.listen(port, host, () => { | |||
httpServer.removeListener('error', onError) | |||
resolve(port) | |||
if (host === 'localhost') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should listen to 0.0.0.0
first and then listen to the specified host to avoid other process to listen to that port between switching the host.
Also I guess we need to check 0.0.0.0
for hosts other than localhost too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi sapphi-red, I have a fix for your suggestion here: Scttpr#3 I am unsure how I should move forward with this
fix: use try/catch block for error handling to avoid chaining.
There was an update but I have checked, it is possible to have an ipv6 and an ipv4 process running on the same port. Node looks to respect this behavior thus I'm not sure this check is needed finally. |
Description
This PR is solving issue 10638.
After a short investigation, recent NodeJS versions will not detect running processes on
0.0.0.0:<desired_port>
if the provided host islocalhost
but it will if hostname resolves to127.0.0.1
.This PR is just changing the default secure hostname from
localhost
to127.0.0.1
without changing anything about custom name resolved by #8647.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).