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

TcpListener::bind_to_port and related changes #23161

Closed
wants to merge 3 commits into from

Conversation

stepancheg
Copy link
Contributor

  • add IPV6_V6ONLY setsockopt constant
  • use it on Windows on listening sockets, because on Windows, unlike other operating systems, socket listening on IPv6 doesn't accept IPv4 connections by default
  • add bind_to_port shortcut to hide all those details from rust users

Note, I didn't try code on Windows. Only tried on OSX with if cfg!(windows) line commented out, in which case setsockopt is called and did nothing, but at least, it didn't fail, and tests passed.

IPV6_V6ONLY is false by default on Linux and OSX (checked myself), and
true by default on Windows [1].

Windows set this option to true for backward compatibility, but rust has
no that requirement, and it is important for rust have TcpListener code
work same way on all platforms.

Didn't try code on Windows, however, I tried it on OSX with
`cfg!(windows)` commented out.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms738574%28v=vs.85%29.aspx
`bind_to_port(port)` is just a shortcut to `bind(&("::", port))`.

However, it is important to have this function to emphasize that
returned socket listens on both IPv4 and IPv6.
@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks for the PR! Changes like this, however, should go through the RFC process instead of being made through a PR first. We do plan on allowing advanced configuration of sockets other than the high level convenience constructors of TcpListener::bind for example. I also don't mind creating an RFC issue to start tracking this sort of implementation as I'd love to start work on it sooner rather than later!

@stepancheg
Copy link
Contributor Author

@alexcrichton could you please explain what exactly should I do? Create an issue in rust-lang/rfcs/? Write a new RFC or patch existing RFC 517?

Shouldn't I submit a new PR with first two patches of this PR? I. e. patches that just unify bind behavior on Windows and other OSes.

@alexcrichton
Copy link
Member

To add these now an RFC would be written, but I would recommend creating a "std::net wishlist issue" along the lines of that for std::fs, we may consolidate these at one point into another umbrella issue but it's good to keep track of feature requests for std::net.

I don't think that we want the second patch for now, however. We've been recently moving a bit away from that sort of compatibility across OSes. For example the code to explicitly prevent opening a directory was removed along with special Windows-specific code to remove readonly files. We've been moving more in a direction that the implementation of the underlying primitive is clear and easy to understand without having any form of "magic" happening under the hood (e.g. setting some options only on some platforms). I do understand that this make is such that the precise behavior is not cross platform, but that is somewhat of an inevitability.

@stepancheg
Copy link
Contributor Author

I've created a long list of issues.

@alexcrichton
Copy link
Member

Thanks!

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.

4 participants