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 #12182 - update default port when server is created #12201

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davidstevens37
Copy link

@davidstevens37 davidstevens37 commented Jun 27, 2024

What does this PR do?

This PR sets the default port to 0, which will allow the OS to assign an available ephemeral port when the server is not instructed to use a specific port resolving

Fixes #12182

  • Code changes

How did you verify your code works?

  • I wrote automated tests
  • I ran my tests locally and they pass (build/bun-debug test test/js/node/http/node-http.test.ts)
✓ node:http > createServer > should use the provided port [2.02ms]
✓ node:http > createServer > should assign a random port when undefined [1.47ms]

@davidstevens37 davidstevens37 changed the title fix(#12182): update default port when server is created Fixes #12182 - update default port when server is created Jun 27, 2024
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

We cannot change this default in server.zig (nor should we), as that is a breaking change. If you’d like to change this to match node behavior, then it should be changed in node:http

@davidstevens37
Copy link
Author

@Jarred-Sumner got it, thanks! Sorry, about that. I wasn't aware that was the default node behavior, just saw an open issue and wanted to support. Love what you're doing here! Just updated it in node:http instead.

Also, any chance you're able to point me in the right direction for #7716? I don't know a thing about Zig, but could be something fun to dig into with when i've got some downtime.

Copy link
Contributor

@Electroid Electroid 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 submitting a PR! I leave some minor comments on the tests to make them less flaky.

test/js/node/http/node-http.test.ts Outdated Show resolved Hide resolved
test/js/node/http/node-http.test.ts Outdated Show resolved Hide resolved
@davidstevens37 davidstevens37 force-pushed the fix-12182/update_default_server_port branch from c3e8e9a to cb43ad3 Compare June 27, 2024 18:03
@davidstevens37 davidstevens37 force-pushed the fix-12182/update_default_server_port branch from cb43ad3 to 292b1a4 Compare June 27, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http.Server.listen(undefined) uses 3000
3 participants