Err(Timeout(Global)) can be returned even when no timeouts are set

[al13n321]

https://docs.rs/ureq/3.0.0-rc4/ureq/enum.Error.html#variant.Timeout says:

By default no timeouts are set, which means this error can’t happen.

But it can. E.g. in src/unversioned/transport/tcp.rs:

    let maybe_stream = if let Some(when) = timeout.not_zero() {
        TcpStream::connect_timeout(&addr, *when)
    } else {
        TcpStream::connect(addr)
    }
    .normalize_would_block();

    let stream = match maybe_stream {
        Ok(v) => v,
        Err(e) if e.kind() == io::ErrorKind::TimedOut => {
            return Err(Error::Timeout(timeout.reason))
        }
        Err(e) => return Err(e.into()),
    };

I.e. if the OS-dependent socket implementation returned ErrorKind::TimedOut after some environment-dependent timeout, ureq translates it to Error::Timeout(something) regardless of the configured timeouts.

In my test it returned Timeout(Global) (because timeout had reason = Global, I guess because it's the default when no timeouts are set?). That's particularly confusing because I would expect the global timeout to be a ureq-level concept, the least platform- or environment-dependent of all timeouts.

A guess at what to change (I'm not familiar with ureq, feel free to ignore):

• If the code above, if the TcpStream::connect(addr) branch returned ErrorKind::TimedOut, report it as Timeout::Connect instead of timeout.reason. Do a similar change to all other code sites that handle io::Error.
• Update https://docs.rs/ureq/3.0.0-rc4/ureq/enum.Error.html#variant.Timeout to say that timeouts may happen even if not explicitly configured, as the OS-es usually (always?) have some timeouts for socket operations.
• Update https://docs.rs/ureq/3.0.0-rc4/ureq/config/struct.ConfigBuilder.html#method.timeout_connect and other timeout_* to say that None means use the OS's default timeout for the corresponding operation, ideally listing the operations (e.g. connect() syscall in case of timeout_connect, at least on Linux?) (if not super many or super platform dependent? idk).

[algesten]

@al13n321 wouldn't it be better to make a new TimeoutReason value like External that we only use if the timeout is not configured and we still get a timeout?

[al13n321]

That was my first thought as well. But if the user sets timeout_connect to 1 hour, TcpStream::connect_timeout would still return io::ErrorKind::TimedOut after the same ~2 minutes. It seems strange to return different error depending on whether the the timeout in Config was set to "pretty much infinite" vs "literally infinite". In both cases the effective timeout is min(timeout from ureq Config, timeout from the OS).

(From https://doc.rust-lang.org/src/std/sys/pal/unix/net.rs.html#168 , it looks like TcpStream::connect_timeout doesn't change any socket options, so it can't make timeout longer. And it reports the same ErrorKind when hitting the requested timeout (i.e. the socket didn't become writable in time) vs getting ETIMEDOUT from the socket (which is not affected by the timeout parameter).)

[Disclaimer: I learned most of the below today, don't trust it too much.]

AFAIU:

• Linux's connect timeout is usually around 1-2 minutes and can't be changed per socket.
• Linux's send/recv will also give up after a few minutes if the connection is completely dead, and this also can't be changed per socket (except if it's affected by keepalive? idk how exactly that works).

So my updated mental model is: the OS will eventually give up on a completely dead connection, and this behavior mostly can't be configured per socket; separately, the user of the socket is free to give up earlier and just close the socket.

So it would make sense to have separate TimeoutReason-s for when the OS gave up on the TCP connection vs when ureq gave up on the request. But wrappers like TcpStream::connect_timeout don't make it easy as they discard this information. Maybe not worth the complexity.

So I guess documentation can say something to the effect:

• For timeout_connect: the effective timeout is min(timeout_connect, timeout in the OS). The timeout in the OS is usually tens of seconds on Windows, 1-2 minutes on Linux.
• For timeout_recv_*, timeout_send_*: regardless of this timeout, the request will also fail if the OS gives up on the TCP connection, which usually happens if it's unresponsive (not just idle) for a few minutes.

[algesten]

That was my first thought as well. But if the user sets timeout_connect to 1 hour, TcpStream::connect_timeout would still return io::ErrorKind::TimedOut after the same ~2 minutes. It seems strange to return different error depending on whether the the timeout in Config was set to "pretty much infinite" vs "literally infinite". In both cases the effective timeout is min(timeout from ureq Config, timeout from the OS).

Good point!. I guess we can never be sure about the actual reason for a timeout, because the error causes are normalized together.

So I guess documentation can say something to the effect:

I agree those are good doc updates.

Not saying we should do it, but one thing to consider is threads. For DNS lookups, std library doesn't give us any timeout options. This is solved via a thread. This could potentially be generalized into a method we could use in more timeout cases.

And yet another random thought. There are plans to use socket2 for other things, maybe there are things we can use in there?

In the socket2 doc there is interesting doc for connect_timeout()

Unlike many other methods on Socket, this does not correspond to a single C function. It sets the socket to nonblocking mode, connects via connect(2), and then waits for the connection to complete with poll(2) on Unix and select on Windows. When the connection is complete, the socket is set back to blocking mode. On Unix, this will loop over EINTR errors.

This could potentially be yet another avenue we might want to consider. Turning all or some of our blocking calls into non_blocking + busy/waiting (polling). If the poll timeout is long enough (1ms?) it might not be a huge impact on the system, and still not cause excessive latency. Maybe there are some blocking call timeouts that would benefit more from this approach than others?

[al13n321]

In the socket2 doc there is interesting doc for connect_timeout()

That's exactly the same as what TcpStream does: https://doc.rust-lang.org/src/std/sys/pal/unix/net.rs.html#168 . The code looks like it's copy-pasted from there.

Turning all or some of our blocking calls into non_blocking + busy/waiting (polling).

Not sure what you mean. poll() is not busy-waiting, it blocks until either the fd becomes ready or the timeout elapses. AFAIK, socket API doesn't have any unavoidable blocking (unlike getaddrinfo) and doesn't require busy-waiting (there's poll/select, there's blocking send/recv with timeouts configurable using setsockopt(SO_SNDTIMEO/SO_RCVTIMEO)). I can't think of any use for busy-wait in ureq.

I didn't fully check, but socket2 would make no difference for socket connect/send/recv. TcpStream already does the straightforward reasonable sequence of syscalls (connect+poll, setsockopt(SO_SNDTIMEO/SO_RCVTIMEO), send, recv). For configuring keepalive it makes sense, strange that the standard library doesn't have it. (Alternatively, ureq could directly call setsockopt on TcpStream's as_raw_fd, on unix. And presumably some winapi function on as_raw_socket on windows. This probably covers all platforms, but I'm not sure.)

[algesten]

Not sure what you mean. poll() is not busy-waiting, it blocks until either the fd becomes ready or the timeout elapses. AFAIK, socket API doesn't have any unavoidable blocking (unlike getaddrinfo) and doesn't require busy-waiting (there's poll/select, there's blocking send/recv with timeouts configurable using setsockopt(SO_SNDTIMEO/SO_RCVTIMEO)). I can't think of any use for busy-wait in ureq.

Oh, sorry, I didn't realize poll is blocking with a timeout.

I didn't fully check, but socket2 would make no difference for socket connect/send/recv. TcpStream already does the straightforward reasonable sequence of syscalls (connect+poll, setsockopt(SO_SNDTIMEO/SO_RCVTIMEO), send, recv).

If we relied on poll() instead of SO_SNDTIMEO/SO_RCVTIMEO, wouldn't that help us tell apart a timeout from the OS vs our own timeout?

For configuring keepalive it makes sense, strange that the standard library doesn't have it. (Alternatively, ureq could directly call setsockopt on TcpStream's as_raw_fd, on unix. And presumably some winapi function on as_raw_socket on windows. This probably covers all platforms, but I'm not sure.)

We kind pride ourselves on having #![forbid(unsafe_code)], though, so any direct manipulation of the fd would need to be via some crate. Maybe there is a more lightweight alternative to socket2? And that crate would need to abstract over unix/windows differences – maybe that takes us all the way back to socket2.