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

fix: already in use port #10638 #10651

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

Scttpr
Copy link

@Scttpr Scttpr commented Oct 26, 2022

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 is localhost but it will if hostname resolves to 127.0.0.1.

This PR is just changing the default secure hostname from localhost to 127.0.0.1 without changing anything about custom name resolved by #8647.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it. -> It passes anyway but I updated unit tests

@Scttpr Scttpr changed the title Fix: already in use port (fix #10638[,#10638]) Fix: already in use port (fix #10638[https://github.com/vitejs/vite/issues/10638]) Oct 26, 2022
@Scttpr Scttpr changed the title Fix: already in use port (fix #10638[https://github.com/vitejs/vite/issues/10638]) Fix: already in use port (fix #10638) Oct 26, 2022
@Scttpr Scttpr changed the title Fix: already in use port (fix #10638) Fix: already in use port #10638 Oct 26, 2022
@Scttpr Scttpr changed the title Fix: already in use port #10638 fix: already in use port #10638 Oct 26, 2022
Copy link
Member

@sapphi-red sapphi-red 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 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')

. 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.

@Scttpr
Copy link
Author

Scttpr commented Oct 27, 2022

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')`

. 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.

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 ?

@thomasballinger
Copy link

(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') {
Copy link
Member

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.

Copy link

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

@sapphi-red sapphi-red linked an issue Oct 30, 2022 that may be closed by this pull request
4 tasks
@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Oct 30, 2022
fix: use try/catch block for error handling to avoid chaining.
@Scttpr
Copy link
Author

Scttpr commented Nov 10, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check 0.0.0.0 as well as localhost/127.0.0.1 when checking for used port
6 participants