RFC: Socket timeouts

[aturon]

Add sockopt-style timeouts to std::net types.

Rendered

[aturon]

cc @sfackler @carllerche @alexcrichton

[carllerche]

Seems good and simple.

• Wait for Duration to be stable. Socket timeouts can be implemented in user land, so there is no huge rush
• There should be a way to get the current timeout as well.
• I'm not too concerned with drawback. Abstractions that accept a reader should not care about timeouts. Timeouts are an ops level concern and should probably be specified at the "edge". If an abstraction needs to set a timeout, it should probably take ownership of the socket.

[reem]

cc me, @seanmonstar

[seanmonstar]

typo innter

[sfackler]

👍 Though I agree with @carllerche that we'll want getters too.

It might be worth putting the methods on a Timeout trait. I ended up defining my own when rust-postgres was on oldio since it needs to abstract over TCP and Unix sockets: https://github.com/sfackler/rust-postgres/blob/df4d5d8417ddd1f421388aefb1d09f266e5168c3/src/io.rs#L15-L30

[sfackler]

It does feel moderately janky to use Duration::zero() to unset the timeout. Should the argument be an Option<Duration> instead? None and Some(Duration::zero()) would end up doing the same things, but that's not the end of the world.

[netvl]

What about connection timeouts? They are usually as important as read timeouts, if not more.

[carllerche]

@netvl I agree, but that is going to probably require a new RFC as there is currently no way to "configure" socket connection.

[seanmonstar]

I'd also like to see if we can stabilize a set_keepalive function. Not quite the same, but closely related. If it doesn't make sense in this RFC, I can file a new one.

[alexcrichton]

@netvl, @carllerche unfortunately a connection timeout is much more complicated than a socket timeout. The options being bound in this RFC have clear implementations on all platforms and have pretty obvious semantics. Connecting with a timeout, however, involves creating a socket, setting it in nonblocking mode, trying to connect, waiting for it to finish, and then finally setting back to blocking mode before returning it.

Regardless this sort of functionality would definitely be done through a "socket builder" type instead of the current convenience functions to create sockets if it is to be implemented.

[alexcrichton]

@sfackler my personal thought was that because None and Some(Duration::zero()) did the same thing using an Option as an argument was a little misleading. For example some might think that Some(Duration::zero()) could be a method to emulate non-blocking mode (aka time out if there's no data immediately), which isn't what actually happens at the system layer.

[l0kod]

👍 for a Timeout trait.

[aturon]

I've updated the RFC to be more explicit about the socket options, to add getter methods, and to lay out a possible alternative for using Option<Duration> instead. I'm especially eager for feedback on that last point -- otherwise I think there's largely consensus for doing at least this much. (Traits can come later on.)

[alexcrichton]

I discussed this on IRC briefly, but we may want to specify the truncation behavior here of timeouts a bit specially. The duration RFC recommends truncating in all cases, but this means that on Windows when you specify a 5ns timeout you would actually pass down a 0ms timeout, which is then interpreted as an infinite timeout.

I'd personally err on the side of something like:

• If the Duration specified is of 0-length, skip the next few steps.
• If the Duration specified cannot be exactly specified to the current platform, it is truncated to the largest Duration smaller than the given argument which can be exactly specified.
• If this Duration now has a length of 0, the smallest nonzero duration is then specified.

Morally speaking <1ms timeouts on Windows will be rounded up, but all other timeouts will be rounded down. In a sense this kinda makes sense because 0 in this context represents infinity and rounding down to infinity seems odd, so we just do the next best thing!

[carllerche]

A Duration of 0 meaning infinite timeout is a bit unfortunate and I think that rust can do better. I believe that in the original RFC, Option<Duration> was suggested. I don't recall why this was not approved, but it seems that it would fix the "duration 0 == infinite" problem.

[nagisa]

This round-down approach is pretty backwards, given there’s at least one API that is supposed to take at least specified amount of time.

Timeout of 0 associates to non-blocking operations, rather than infinity to me. So, when you pass timeout of 0, you either get timeout/EWOULDBLOCK error or something useful happens immediately.

For removing timeouts remove_timeout() or set_timeout(None) makes much, much more sense. Alternatively, special-casing Duration representation to mean infinity if all bits are set to 1 could be possible.

[alexcrichton]

@carllerche

I don't recall why this was not approved, but it seems that it would fix the "duration 0 == infinite" problem.

As outlined in the alternative the problem with this approach is that Some(Duration::zero()) is equivalent to "infinite timeout" which may be counter-intuitive.

---

@nagisa

This round-down approach is pretty backwards, given there’s at least one API that is supposed to take at least specified amount of time.

Another alternative @aturon and I were talking about (which I think was omitted from the alternatives by accident) was to return an error on durations such as this. For example specifying 5ns on windows would return an error. This would also possibly open the door to returning an error on Some(0) if we used Option<Duration> instead.

[andrew-d]

Just throwing this out there, but: is there any reason we can't define Duration as something like:

enum Duration {
    None,
    Infinite,
    Some(u64 /* Or whatever */),
}

Where Some and None can be changed to names that don't conflict with Option, of course.

[aturon]

@alexcrichton IIRC we discussed only making it an error to send a Some(Duration::zero()), and I think that giving an error for non-zero durations that would happen to round down on specific platforms is much too subtle. I would personally vote for rounding up in this case.

With that said: is there anyone who strongly prefers to take Duration rather than the proposed alternative of Option<Duration> (and returning an invalid input error on zero)? If not, I will switch around the proposals in the RFC.

[aturon]

I have updated the RFC to make Option<Duration> the primary proposal.

The question of rounding for Windows is left as an implementation detail, somewhat dependent on how things work out with Duration in general.

This RFC should be considered to be in its "final comment period" at this point.

[sfackler]

Will this return None if there's an error calling getsockopt?

[aturon]

Argh, good question! I totally failed to consider errors here. Probably we want io::Result<Option<Duration>>.

[aturon]

cc @alexcrichton, I imagine you don't like that idea.

[alexcrichton]

You are correct! :)

I'd be fine with io::Result<Duration> where some error is returned for a nonexistent duration. (e.g. ErrorKind::NotFound)

[sfackler]

That seems like pretty weird overloading of concepts - I would not consider no timeout to be an error since it's the default case!

Is Result<Option<Duration>> really that bad? I'd assume that 99% of users are going to let timeout = try!(foo.write_timeout()); so won't even run into the double indirection.

[seanmonstar]

Yea, having try!(s.read_timeout()) return an error for no timeout would be confusing. There wasn't an error reading it! Result<Option<Duration>> is the correct signature in this case.

[alexcrichton]

I personally see Result<Option<Duration>> as quite bad (too much nesting). I re-consulted the documentation for getsockopt and it states

Note that not all implementations allow this option to be retrieved.

This means that a successful set_write_timeout may not be followed by a successful write_timeout so you're probably not just try!-ing away the error in the robust case anyway.

[seanmonstar]

The way it would be represented in other languages matches Result<Option<Duration>>. For instance, Python would be:

try:
  timeout = so.gettimeout()
  if timeout is not None:
    # whatever
except OSError, e:
  # handle error

[nagisa]

Some(Duration::new(0, 0)) can be mostly back-compatibly be mapped to mark the socket non-blocking at a later time, though.

[aturon]

I've pushed an update returning Result<Option<Duration>>, but also laying out an alternative enum-based approach that could handle nonblocking support in the same API.

[seanmonstar]

Is this good to merge now? My calculations suggest that this missed the 1.1 train :(

[aturon]

@seanmonstar It's been in it's final comment period for two weeks, so it seems good to me. (We're still getting the subteams fully up and running.)

@alexcrichton thoughts?

[nagisa]

Before this lands, I’d really want for set_timeout(Some(Duration::new(0, 0))) mean set_blocking(false) (see #1047 (comment); and of course, I want opinions on that too)

[alexcrichton]

I agree with @aturon that I believe this RFC has baked long enough that this is ready for acceptance. There is broad support for the API proposed here as well as the semantics of what'll happen on Windows and Unix, and the case @nagisa brought up for nonblocking sockets is a backwards-compatible extension that can be made (and the implementations will initially land as #[unstable]). After giving more thought to socket options in general this API also fits quite nicely into the current proposed conventions for further socket options.

As a result, I'm going to merge this and open an issue for tracking the implementation, thanks for the comments everyone!

[seanmonstar]

Not sure where ongoing discussion of this should go, so trying here.

Currently, after setting a timeout, if a read does trigger a timeout, the error returned is e.kind() == WouldBlock. Would it make sense to internally match for WouldBlock, and if a timeout for the current operation has been set, and/or the socket is not in non-blocking mode, translate the error into TimedOut?

/cc @softprops

[sfackler]

I considered that during the implementation, but we decided against that under the rationale that it was too magical - io::Error maps directly down to errno and seems a bit weird to change that in random cases. Checking if the socket is in non-blocking mode/has a timeout would each be a system call, which seems like it would violate the "direct bindings" philosophy of the current IO module. For example, a call to TcpStream::read would change from simply a call to libc read to a call to read and then maybe a couple of fnctl calls if the read timed out.

It might make sense to add a method to io::ErrorKind that would match against both WouldBlock and TimedOut to make handling it more straightforward.

[carllerche]

Also, it's very possible to take a std::io socket and switch it to non blocking, which would then return WouldBlock when the socket is not ready (vs. a timeout).

[softprops]

It might make sense to add a method to io::ErrorKind that would match against both WouldBlock and TimedOut to make handling it more straightforward.

There was an interesting conditional in a test in the pr that seemed like it hinted at the need for that

https://github.com/rust-lang/rust/pull/25818/files#diff-e1b6aa0742f9015d1840084d1925ece9R941

[seanmonstar]

@carllerche yea, thus the suggestion of checking if the socket is non-blocking.

@sfackler it wouldn't be the first time that an API was normalized because of OS differences: On Windows, a read return value of -1 will first be checked for WSAESHUTDOWN, to satisfy Rust's API contract of returning Ok(0) at EOF.