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.

Sign up for GitHub

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

Jump to bottom

Allow the dev server socket to be reused immediately #4709

Merged
merged 2 commits into from
Apr 26, 2023
Merged

Allow the dev server socket to be reused immediately #4709

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec92d73
Allow dev server socket to be reused immediately
alexkirsz Apr 26, 2023
a393b56
disable reuseaddr on windows
sokra Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/turbopack-dev-server/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ impl DevServer {
// real TCP listener, see if it bound, and get its bound address.
let socket = Socket::new(Domain::for_address(addr), Type::STREAM, Some(Protocol::TCP))
.context("unable to create socket")?;
// Allow the socket to be reused immediately after closing. This ensures that
// the dev server can be restarted on the same address without a buffer time for
// the OS to release the socket.
// https://docs.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse
#[cfg(not(windows))]
let _ = socket.set_reuse_address(true);
Copy link
Member

Choose a reason for hiding this comment

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

This will also allow binding to the same port twice, no?

Copy link
Member

Choose a reason for hiding this comment

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

pretty sure this will break our find_port function
https://github.com/vercel/next.js/blob/3bdaed01ba9e1a86a16fd243e5bdb5e4894ed5aa/packages/next-swc/crates/next-dev/src/lib.rs#L173-L173

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's SO_REUSEPORT.

Copy link
Member

Choose a reason for hiding this comment

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

This indicates that futher calls to bind may allow reuse of local addresses. For IPv4 sockets this means that a socket may bind even when there's a socket already listening on this port

from the rustdoc

Copy link
Member

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse#using-so_reuseaddr

also windows docs

Copy link
Member

Choose a reason for hiding this comment

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

and it also says linux now matches this behavior

I'm fine with merging this for macOS/Linux only, but we might have to update the port finding code, we've already had a bunch of bug reports because of it not reporting ports in use correctly / not finding a new port as expected in the past and this might make it worse

Copy link
Member

Choose a reason for hiding this comment

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

this may very well break find_port on macos and linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test find_port on macOS with and without this option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

find_port still works properly with this option on macOS. However, as you said, binding to different local addresses no longer produces an error.

One way to fix this would be to replace find_port to run lsof/other OS-specific commands to check if a port is in use instead of trying to bind to that port.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to match the old behavior from before #4565: https://github.com/rust-lang/rust/blob/9c044d77a334609513f3b615e0763a40cc02424d/library/std/src/sys_common/net.rs#L399-L407

if matches!(addr, SocketAddr::V6(_)) {
// When possible bind to v4 and v6, otherwise ignore the error
let _ = socket.set_only_v6(false);
Expand Down