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

Implement RFC 1461 #31945

Merged
merged 6 commits into from
Mar 4, 2016
Merged

Implement RFC 1461 #31945

merged 6 commits into from
Mar 4, 2016

Conversation

sfackler
Copy link
Member

I have these tagged as stable in 1.9, so this shouldn't merge until the 1.8 beta's cut.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@sfackler
Copy link
Member Author

Works on Windows now.

I ended up backing out the addition of TcpStream::{set_,}keepalive. There's actually two operations going on here - enabling keepalive and changing the interval from the system default. In addition, you can't load the interval on Windows, so TcpStream::keepalive doesn't even work there (it'll always return Ok(None)).

We should probably split the keepalive functionality into two methods to mirror how things work, and possibly move the interval accessor to a unix-only extension trait, but that should be worked out in the net2 crate.

/// Sets the value for the `IPV6_V6ONLY` option on this socket.
///
/// If this is set to `true` then the socket is restricted to sending and
/// receiving IPv6 packets only. In this case two IPv4 and IPv6 applications
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case two IPv4 and IPv6 applications can bind the same port at the same time

This sounds somewhat misleading. Wouldn't that be one of each?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Also back out keepalive support for TCP since the API is perhaps not
actually what we want. You can't read the interval on Windows, and
we should probably separate the functionality of turning keepalive on
and overriding the interval.

t!(stream.set_nonblocking(true));
t!(stream.set_nonblocking(false));
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a read here to make sure it doesn't block?

@alexcrichton
Copy link
Member

Thanks @sfackler! 1.8 has been branched, so feel free to r=me with a few nits addressed

@sfackler
Copy link
Member Author

sfackler commented Mar 3, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

📌 Commit 631fa2b has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

⌛ Testing commit 631fa2b with merge f1eccfb...

@bors
Copy link
Contributor

bors commented Mar 3, 2016

💔 Test failed - auto-linux-64-x-android-t

@sfackler
Copy link
Member Author

sfackler commented Mar 3, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

📌 Commit c8eb1e2 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

⌛ Testing commit c8eb1e2 with merge 01cdce9...

@sfackler
Copy link
Member Author

sfackler commented Mar 3, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

📌 Commit ee62aab has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

⌛ Testing commit ee62aab with merge f7f9d8a...

@bors
Copy link
Contributor

bors commented Mar 3, 2016

💔 Test failed - auto-linux-cross-opt

@sfackler
Copy link
Member Author

sfackler commented Mar 3, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 3, 2016

📌 Commit e4aa513 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 4, 2016

⌛ Testing commit e4aa513 with merge 8e261d1...

bors added a commit that referenced this pull request Mar 4, 2016
I have these tagged as stable in 1.9, so this shouldn't merge until the 1.8 beta's cut.
@bors bors merged commit e4aa513 into rust-lang:master Mar 4, 2016
@sfackler sfackler added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 11, 2016
@sfackler sfackler deleted the net2 branch November 26, 2016 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants