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

Tracking issue for the socket timeout functions #27773

Closed
alexcrichton opened this issue Aug 12, 2015 · 13 comments · Fixed by #28339
Closed

Tracking issue for the socket timeout functions #27773

alexcrichton opened this issue Aug 12, 2015 · 13 comments · Fixed by #28339
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@alexcrichton
Copy link
Member

This is a tracking issue for the unstable socket_timeout feature in the standard library. Now that the Duration type is stable it may be the case that these methods can just be stabilized as-is.

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@alexcrichton
Copy link
Member Author

cc @sfackler

@alexcrichton alexcrichton added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 13, 2015
@alexcrichton
Copy link
Member Author

This feature is now entering its final comment period for stabilization.

@seanmonstar
Copy link
Contributor

It was pointed out in a hyper issue that the returned error when a timeout is hit is OS dependent. It might be helpful to include a Errors subheading to the documentation of these methods?

  • Unixy returns ErrorKind::WouldBlock
  • Windows returns ErrorKind::TimedOut

An alternative would be to normalize them into 1 type, but that seems more controversial.

@reem
Copy link
Contributor

reem commented Aug 14, 2015

I would like them to be normalized into one kind of error, not doing so will make code that uses these APIs non-cross-platform by default, which is very unfortunate IMO.

@eefriedman
Copy link
Contributor

It's actually worse than the error codes just being inconsistent: on Windows, recv() returns WSAETIMEDOUT both when the receive call times out and when the connection itself times out, so you can't tell if the connection is still alive.

@droundy
Copy link
Contributor

droundy commented Sep 3, 2015

What does it mean for a socket itself to time out?

On Thu, Sep 3, 2015, 4:38 AM eefriedman notifications@github.com wrote:

It's actually worse than the error codes just being inconsistent: on
Windows, recv() returns WSAETIMEDOUT both when the receive call times out
and when the socket itself times out, so you can't tell if the socket is
still alive.


Reply to this email directly or view it on GitHub
#27773 (comment).

@eefriedman
Copy link
Contributor

Err, sorry, I meant the connection, not the socket.

@sfackler
Copy link
Member

sfackler commented Sep 3, 2015

Is the connection timing out in reference to TCP keepalive or something?

@alexcrichton
Copy link
Member Author

@seanmonstar, @reem, my personal inclination is to do what we do in the rest of std::io, which is to pass through exactly what the system does in as many places as possible. I've found that tampering with things like the error code and trying to unify them across platforms generally leads to weird corner cases which act differently than you may expect, so just reexposing what's happening underneath has worked quite well for us so far.

@eefriedman
Copy link
Contributor

Is the connection timing out in reference to TCP keepalive or something?

It's a bit more general than that: it means that the network stack closed the connection because it couldn't get a response from the other end. That doesn't necessarily mean a keepalive; it could also mean that it hit the general retransmission limit.

@alexcrichton The problem here isn't precisely the portability of error codes; it's that recv() on Windows claims the connection is dead when it actually isn't. This means portable code must assume the connection is dead if the read timeout expires, so people will write code on Unix and find that there's no way to replicate the behavior on Windows without switching from std::net to some other networking library.

@aturon
Copy link
Member

aturon commented Sep 9, 2015

@alexcrichton

my personal inclination is to do what we do in the rest of std::io, which is to pass through exactly what the system does in as many places as possible

I'm not sure I agree with that characterization, though I know we had many debates about how to handle errors :-) In particular, though, we strove for cross-platform behavior wherever reasonable while also avoiding laying on extra semantics or overhead. Of course, the hard cases are when these constraints are at odds.

From my perspective, if there's a reasonably clear way to merge or reinterpret errors so that they are consistent across platforms without information loss, I prefer doing so. The downside is that it's more difficult to map back to the underlying system error. (IIRC we ended up having a single mapping function from system errors to our IO errors, so what I'm suggesting is indeed somewhat of a departure.)

Failing that, it's not too bad if there are multiple errors you might get depending on the platform, as long as there's a reasonable, robust way to program against them that doesn't require #[cfg].

It's not clear whether we can achieve either of these, though, without layering on some extra checking/semantics on the Windows side -- but I haven't dug into the specifics too much.

Can someone more familiar with the details propose an alternative that favors cross-platform semantics?

@eefriedman
Copy link
Contributor

Alternative: don't use SO_RCVTIMEO or SO_SNDTIMEO because the semantics aren't portable. (Even if you ignore the Windows issues, the semantics are slightly different on Linux vs. *BSD.) Instead, use non-blocking sockets and select() to implement the intended functionality.

@aturon
Copy link
Member

aturon commented Sep 11, 2015

We discussed the issues in this thread fairly extensively in the last libs team meeting. The consensus was to follow the philosophy laid out in @alexcrichton's earlier comment: rather than trying to layer on additional semantics for cross-platform compatibility, we optimize for transparency, where IO functionality has a clear mapping to system APIs -- including errors. That means, in particular, sticking with a single global mapping of OS error codes to our IO error kinds.

This strategy is contingent on clear documentation of platform-specific behavior, and the ability to provide additional layers, in std and elsewhere, that try to provide higher-level bridges between platforms. In particular, @sfackler suggested that the ErrorKind enum (or perhaps the Error type itself) could provide helper methods for deciphering errors in a context-specific and more cross-platform way. This provides an opt-in semantic layering over the raw system API. We plan to prototype this approach out of tree, and then consider bringing it into std.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 13, 2015
The FCP is coming to a close and 1.4 is coming out soon, so this brings in the
libs team decision for all library features this cycle.

Stabilized APIs:

* `<Box<str>>::into_string`
* `Arc::downgrade`
* `Arc::get_mut`
* `Arc::make_mut`
* `Arc::try_unwrap`
* `Box::from_raw`
* `Box::into_raw`
* `CStr::to_str`
* `CStr::to_string_lossy`
* `CString::from_raw`
* `CString::into_raw`
* `IntoRawFd::into_raw_fd`
* `IntoRawFd`
* `IntoRawHandle::into_raw_handle`
* `IntoRawHandle`
* `IntoRawSocket::into_raw_socket`
* `IntoRawSocket`
* `Rc::downgrade`
* `Rc::get_mut`
* `Rc::make_mut`
* `Rc::try_unwrap`
* `Result::expect`
* `String::into_boxed_slice`
* `TcpSocket::read_timeout`
* `TcpSocket::set_read_timeout`
* `TcpSocket::set_write_timeout`
* `TcpSocket::write_timeout`
* `UdpSocket::read_timeout`
* `UdpSocket::set_read_timeout`
* `UdpSocket::set_write_timeout`
* `UdpSocket::write_timeout`
* `Vec::append`
* `Vec::split_off`
* `VecDeque::append`
* `VecDeque::retain`
* `VecDeque::split_off`
* `rc::Weak::upgrade`
* `rc::Weak`
* `slice::Iter::as_slice`
* `slice::IterMut::into_slice`
* `str::CharIndices::as_str`
* `str::Chars::as_str`
* `str::split_at_mut`
* `str::split_at`
* `sync::Weak::upgrade`
* `sync::Weak`
* `thread::park_timeout`
* `thread::sleep`

Deprecated APIs

* `BTreeMap::with_b`
* `BTreeSet::with_b`
* `Option::as_mut_slice`
* `Option::as_slice`
* `Result::as_mut_slice`
* `Result::as_slice`
* `f32::from_str_radix`
* `f64::from_str_radix`

Closes rust-lang#27277
Closes rust-lang#27718
Closes rust-lang#27736
Closes rust-lang#27764
Closes rust-lang#27765
Closes rust-lang#27766
Closes rust-lang#27767
Closes rust-lang#27768
Closes rust-lang#27769
Closes rust-lang#27771
Closes rust-lang#27773
Closes rust-lang#27775
Closes rust-lang#27776
Closes rust-lang#27785
Closes rust-lang#27792
Closes rust-lang#27795
Closes rust-lang#27797
bors added a commit that referenced this issue Sep 13, 2015
The FCP is coming to a close and 1.4 is coming out soon, so this brings in the
libs team decision for all library features this cycle.

Stabilized APIs:

* `<Box<str>>::into_string`
* `Arc::downgrade`
* `Arc::get_mut`
* `Arc::make_mut`
* `Arc::try_unwrap`
* `Box::from_raw`
* `Box::into_raw`
* `CStr::to_str`
* `CStr::to_string_lossy`
* `CString::from_raw`
* `CString::into_raw`
* `IntoRawFd::into_raw_fd`
* `IntoRawFd`
* `IntoRawHandle::into_raw_handle`
* `IntoRawHandle`
* `IntoRawSocket::into_raw_socket`
* `IntoRawSocket`
* `Rc::downgrade`
* `Rc::get_mut`
* `Rc::make_mut`
* `Rc::try_unwrap`
* `Result::expect`
* `String::into_boxed_slice`
* `TcpSocket::read_timeout`
* `TcpSocket::set_read_timeout`
* `TcpSocket::set_write_timeout`
* `TcpSocket::write_timeout`
* `UdpSocket::read_timeout`
* `UdpSocket::set_read_timeout`
* `UdpSocket::set_write_timeout`
* `UdpSocket::write_timeout`
* `Vec::append`
* `Vec::split_off`
* `VecDeque::append`
* `VecDeque::retain`
* `VecDeque::split_off`
* `rc::Weak::upgrade`
* `rc::Weak`
* `slice::Iter::as_slice`
* `slice::IterMut::into_slice`
* `str::CharIndices::as_str`
* `str::Chars::as_str`
* `str::split_at_mut`
* `str::split_at`
* `sync::Weak::upgrade`
* `sync::Weak`
* `thread::park_timeout`
* `thread::sleep`

Deprecated APIs

* `BTreeMap::with_b`
* `BTreeSet::with_b`
* `Option::as_mut_slice`
* `Option::as_slice`
* `Result::as_mut_slice`
* `Result::as_slice`
* `f32::from_str_radix`
* `f64::from_str_radix`

Closes #27277
Closes #27718
Closes #27736
Closes #27764
Closes #27765
Closes #27766
Closes #27767
Closes #27768
Closes #27769
Closes #27771
Closes #27773
Closes #27775
Closes #27776
Closes #27785
Closes #27792
Closes #27795
Closes #27797
@steveklabnik steveklabnik added this to the 1.3 milestone Sep 18, 2015
@steveklabnik steveklabnik added this to the 1.4 milestone Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants