Strengthen/clarify "It is an error to..." wording

[frewsxcv]

• UdpSocket::set_read_timeout
• UdpSocket::set_write_timeout
• TcpStream::connect_timeout
• TcpStream::set_read_timeout
• TcpStream::set_write_timeout
• UnixDatagram::set_read_timeout
• UnixDatagram::set_write_timeout
• UnixStream::set_read_timeout
• UnixStream::set_write_timeout

For all of the APIs above, the docs have a line with this wording:

It is an error to pass the zero Duration to this method.

My initial thought is, what does "it is an error" mean? Does this mean the API will panic if Some(Duration::from_secs(0)) is passed or will it just return an Err? The answer appears to be the latter.

Also since it's the latter, and we don't have an explicit check for Duration(0) in our codebase, it means we're relying on the behavior being the same across all platforms. Is this alright? Should/can we add an explicit check for Duration(0)? Should we add a test ensuring this is the behavior across all platforms?

Maybe I'm overthinking this...

[sfackler]

We do have an explicit check:

rust/src/libstd/sys/unix/net.rs

Lines 282 to 285 in ff2a7c8

	if dur.as_secs() == 0 && dur.subsec_nanos() == 0 {
	return Err(io::Error::new(io::ErrorKind::InvalidInput,
	"cannot set a 0 duration timeout"));
	}

rust/src/libstd/sys/windows/net.rs

Lines 261 to 264 in ff2a7c8

	if timeout == 0 {
	return Err(io::Error::new(io::ErrorKind::InvalidInput,
	"cannot set a 0 duration timeout"));
	}

[frewsxcv]

@sfackler ah, i didn't see those. i just checked redox and it looks like it doesn't have that conditional. seems like this is prone to happen again in the future, unless we had:

• an integration test suite for std::net things such that we could assert that an Err is returned in this scenario
• or, the conditionals were moved into the body of the top-level entry point of these APIs instead of the platform specific ones

[sfackler]

Yeah it seems reasonable to have tests to check for that error case.

[frewsxcv]

@sfackler for TcpStream::set_read_timeout for example, do you know if we have a way to test this method in our test suite? seems like it would require TcpStream::connect to be called, which seems like it would require a TCP listener running

[sfackler]

I'd just spawn off a listener in a background thread:

let listener = TcpListener::bind("127.0.0.1:0").unwrap();
let addr = listener.local_addr().unwrap();

thread::spawn(move || {
    listener.accept().unwrap();
});

let socket = TcpStream::connect(addr).unwrap();
match socket.set_read_timeout(Some(Duration::from_secs(0))) {
    Err(ref e) if e.kind() == io::ErrorKind::InvalidInput => {}
    _ => panic!("expected error"),
}