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

Port conflict #9133

Closed
1 task
cdtut opened this issue Nov 19, 2023 · 7 comments · Fixed by #9545 or #9661
Closed
1 task

Port conflict #9133

cdtut opened this issue Nov 19, 2023 · 7 comments · Fixed by #9545 or #9661
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)

Comments

@cdtut
Copy link

cdtut commented Nov 19, 2023

Astro Info

Node                     v20.8.0
System                   Linux (x64)

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Start astro dev and it start on 4321. Start astro dev again and it start on next available port 4322.

Run astro preview and there is error EADDRINUSE: address already in use ::1:4321.

Run node dist/server/entry.mjs and it say [@astrojs/node] Server listening on http://127.0.0.1:4321 even though process already running on 4321.

What's the expected result?

  1. astro preview should open on next available port 4322.
  2. node dist/server/entry.mjs should have error EADDRINUSE: address already in use ::1:4321.
  3. astro preview should have command line option --port and astro config file option to change port to try first.
  4. node adapter should have host and port options that are used as default if environment variable not given. Not every build process can provide environment variables (https://docs.astro.build/en/guides/integrations-guide/node/#runtime-environment-variables).
adapter: node({
    host: "0.0.0.0",
    port: "4321"
})

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-1o5lsw

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 19, 2023
@cdtut
Copy link
Author

cdtut commented Nov 20, 2023

There is server.host and server.port which is used by dev server and node adapter. host and port should be possible to pass to node adapter to be different from dev which is needed if running dev and build together.

Another problem is passing host: "0.0.0.0" prints [@astrojs/node] Server listening on http://localhost:4321 but should print 0.0.0.0 instead of localhost. To be consistent with how it's printed in other places Error: listen EADDRINUSE: address already in use 0.0.0.0:4321.

@matthewp matthewp added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged labels Nov 20, 2023
@matthewp
Copy link
Contributor

@bluwy surprising to me, we are fully using Vite for preview server now, right?

@bluwy
Copy link
Member

bluwy commented Nov 21, 2023

We use Vite's preview server for astro preview only (mostly for static sites). The nodejs adapter's preview server is rather simple and doesn't handle it specially, but maybe we're swallowing the EADDRINUSE error somewhere.

@bluwy
Copy link
Member

bluwy commented Dec 29, 2023

  1. astro preview should open on next available port 4322.

When using an adapter, astro preview would route to their implementation, and it's not always possible to expect them to auto-increment the port if it's already occupied. So I don't think this behaviour should be expected by default.

  1. node dist/server/entry.mjs should have error EADDRINUSE: address already in use ::1:4321.

I submitted a PR to fix that #9545. Technically it's not a port conflict behaviour as both servers are served under ipv4 and ipv6 respectively, but it is indeed inconsistent so I updated it.

  1. astro preview should have command line option --port and astro config file option to change port to try first.

--port seems to work for me when I use it.

  1. node adapter should have host and port options that are used as default if environment variable not given. Not every build process can provide environment variables (docs.astro.build/en/guides/integrations-guide/node/#runtime-environment-variables).

I don't think we need new options for those unless you can show an environment where you can't pass env vars today. But I believe most enviroments should allow that in some form. I've skipped this in the PR for now as I think it's also out-of-scope.

@bluwy bluwy self-assigned this Dec 29, 2023
@cdtut
Copy link
Author

cdtut commented Dec 29, 2023

When using an adapter, astro preview would route to their implementation, and it's not always possible to expect them to auto-increment the port if it's already occupied. So I don't think this behaviour should be expected by default.

New option to auto increment is possible? It is for previewing in development without stopping the dev server.

--port seems to work for me when I use it.

I did not know that can it be included to command help menu?

I don't think we need new options for those unless you can show an environment where you can't pass env vars today. But I believe most enviroments should allow that in some form. I've skipped this in the PR for now as I think it's also out-of-scope.

Some companies environment don't let developers customize env vars. It's a limitation they add that makes difficulty.

host and port can already be passed in https://github.com/withastro/astro/blob/main/packages/integrations/node/src/index.ts#L47. Can you move ...userOptions to the end so it can be override?

@bluwy
Copy link
Member

bluwy commented Jan 2, 2024

New option to auto increment is possible? It is for previewing in development without stopping the dev server.

As mentioned, the problem is that different preview implementations are not expected to auto-increment today. Even if we add a new option, it doesn't make auto-increment work. They still need to implement themselves.

I did not know that can it be included to command help menu?

We can definitely add them. Looks like it's missed out in the list.

@cdtut
Copy link
Author

cdtut commented Jan 19, 2024

@ematipico host and port can already be passed in https://github.com/withastro/astro/blob/main/packages/integrations/node/src/index.ts#L47. Can you move ...userOptions to the end so it can be override?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants