Change the timeout API?

[ghost]

Timeouts are confusing. @spacejam recently wrote an example that contains the following piece of code:

stream
    .read_to_end(&mut buf)
    .timeout(Duration::from_secs(5))
    .await?;

The problem here is that we need two ?s after .await and it's easy to forget that.

I think the confusing part is in that the .timeout() combinator looks like it just transforms the future in a similar vein to .map() or .and_then(), but it really does not!

Instead, .timeout() bubbles the result of the future so that its type becomes Result<Result<_, io::Error>, TimeoutError>.

Perhaps it would be less confusing if timeout() was a free-standing function in the future module rather than a method on the time::Timeout extension trait?

future::timeout(
    stream.read_to_end(&mut buf),
    Duration::from_secs(5),
)
.await??;

This timeout() function would stand alongside ready(), pending(), and maybe some other convenience functions in the future module.

Here's another idea. What if we had io::timeout() function that resolves to a Result<_, io::Error> instead of bubbling the results? Then we could write the following with a single ?:

io::timeout(
    stream.read_to_end(&mut buf),
    Duration::from_secs(5),
)
.await?;

Now it's also more obvious that we're setting a timeout for an I/O operation and not for an arbitrary future.

In addition to that, perhaps we could delete the whole time module? I'm not really a fan of it because it looks nothing like std::time and I generally dislike extension traits like Timeout.

Note that we already have a time-related function, task::sleep(), which is not placed in the time module so we probably shouldn't worry about grouping everything time-related into the time module. I think it's okay if we have io::timeout() and future::timeout().

Finally, here's a really conservative proposal. Let's remove the whole time module and only have io::timeout(). A more generic function for timeouts can then be left for later design work. I think I prefer this option the most.

[skade]

I do agree that async_std::time is weird in the sense that it has a different role then the std::time module.

I think the io::timeout and the future::timeout idea is a really strong one.

[yoshuawuyts]

In the original futures post timeouts were done by joining with a timeout. In this case we could get a similar result to io::timeout by using futures::try_join. The other would be covered by join as well.

The only thing that doesn't seem covered so far is how to create a stream of intervals. For any sort of UI loop or intermittent checking this seems incredibly important. Where should it be placed if not in a time submodule? Though I guess this could be emulated by sleeping in a loop also.

In general I don't like the free functions proposed here. I feel we can do better.

[skade]

How about async_std::temporal? time modules are often holding types that represent time (think "9 o'clock" or "5 minutes"). timeouts and interval streams are types representing temporal flow ("triggering 20 seconds from now", "triggering every 20 seconds").

I'm not a huge fan of using very generic methods like try_join for very concrete concepts that should have high visibility. Both timeouts and regular timers should have a clear representation in the API.

I'm also generally not a huge fan of free functions, but in this case, I like that the following works, if we change the argument order:

io::timeout(Duration::from_secs(5), async {
   //.... something
});

[skade]

Sketch code. Another option would be:

struct Timeout {
 //...
}

impl Timeout {
    fn of_duration(d: Duration) -> Self;
    fn on<F: Future>(&self, f: F) { } -> TimeoutFuture;
    fn try<F: Future<Output=Result<O, E>>, O, E>(&self, f : F) {} -> TryTimeout;
}

fn main () {
    let timeout = Timeout::of_duration(Duration::from_secs(5));
    timeout.on(async {});
    timeout.try(async { Ok((()) })
}

This still needs the user to do the right thing, sadly. The underlying problem (no specialization) will persist in any api design.

Note that this allows timeout to be reused and e.g. stored as a config value. Alternatively, it also allows Timeout to also ship a trait that other timeout mechanism implement, but this is something I would externalise. async-std timeouts should be our concrete implementation.

[ghost]

In the original futures post timeouts were done by joining with a timeout. In this case we could get a similar result to io::timeout by using futures::try_join.

You probably meant select instead of join :)

The only thing that doesn't seem covered so far is how to create a stream of intervals. For any sort of UI loop or intermittent checking this seems incredibly important. Where should it be placed if not in a time submodule?

One idea is to do async_std::stream::interval(x). If we already have stream::{empty, once, repeat}, why not also have interval in there?

I kind of want to challenge the assumption that time-related futures and streams should go into the time module. It might be a historical precedent from Tokio that this stuff gets grouped into time, but I wonder if we should embrace that approach.

I just find it jarring because there's nothing in std::time resembling these futures/streams constructors, and see those as something that have to do with futures/streams rather than time. For example, in the futures crate, FutureExt::catch_unwind() is something that exists in the futures::future rather than futures::panic.

[yoshuawuyts]

One idea is to do async_std::stream::interval(x). If we already have stream::{empty, once, repeat}, why not also have interval in there?

@stjepang This is a fantastic idea! I'm fully supportive of this.

---

You probably meant select instead of join :)

Lol oops, yeah. I'm behind a laptop again, and the way I'd see these being used is:

// equivalent to `futures::timeout`.
let res = reg_fut.select(task::sleep(Duration::from_secs(1))?;

// equivalent to `io::timeout`.
let res = io_fut.try_select(task::sleep(Duration::from_secs(1))?;

I'm very sympathetic to @skade's argument that timeouts would benefit from more visibility, and this is def lacking in that department.

---

In general I quite like removing async_std::time, and introducing io::timeout. It seems like that would cover most. Especially @skade's ordering idea seems quite neat!

io::timeout(Duration::from_secs(5), async {
   //.... something
})?;

It's different from the ordering precedent set in, say window.setTimeout, but I think it's reasonable because we don't have any more optional tailing arguments, and I vividly remember being confused by the ordering when learning JS.

[spacejam]

I like stream::interval for tickers.

Could make timeouts restricted on futures where F::Output implements From<std::io::Error> and spit back an ErrorKind::TimedOut, and then you don't need any free-standing functions or cruft, and clean unsurprising usage for things IO related anyway:

impl<F, T> Future for TimeoutFuture<F, T> 
where:
    F: Future,
    F::Output: From<std::io::Result<T>>
{
    type Output = F::Output;

    fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        match self.as_mut().future().poll(cx) {
            Poll::Ready(v) => Poll::Ready(Ok(v)),
            Poll::Pending => match self.delay().poll(cx) {
                Poll::Ready(_) => Poll::Ready(F::Output::from(Err(
                    std::io::Error::new(ErrorKind::TimedOut, "future timed out")
                ))),
                Poll::Pending => Poll::Pending,
            },
        }
    }
}

[sdroege]

What if we had io::timeout() function that resolves to a Result<_, io::Error> instead of bubbling the results?

In addition to the current variant? IMHO the current variant is useful on its own for non-IO futures too. You might have some more complicated user defined futures (which might or might not be around IO futures) that don't return a plain io::Error but some more complex error type (that might contain an io::Error variant somewhere though).

Removing the current variant would seem suboptimal to me.

[DoumanAsh]

Just as example I prefer timeouts that allow me to restart and retrieve original future on failure:

pub fn timed<F: Future>(job: F, timeout: Duration) -> impl Future<Output = Result<F::Output, Expired<F, Timer>>>
And without any extra error from timer itself 😄

[ghost]

@DoumanAsh If fut is a Future, then &mut fut is also a Future, so you can pass a &mut fut to the timeout function and continue using fut if timeout occurred.

Unless I'm missing something, there's not much reason to return the original future since one can use a mutable borrow of the future. Do you agree with that assessment?

[DoumanAsh]

It is indeed true, that with &mut F you could get around it.
But it applies severe restriction on F, it must be Unpin always then, which is not true for async fn and it is not clear whether it will change (Boxing is work around, not a solution)

There is also matter of storing this future, which becomes a user responsibility

[ghost]

But it applies severe restriction on F, it must be Unpin always then

Is that not true also for the case when the timeout function returns the original future back to the user? If F has self-references and gets polled by the timeout function, and is then returned back, returning moves the future and therefore invalidates self-references.

[yoshuawuyts]

The original issue has been resolved, and conversation has settled down. I'm going to go ahead and close this for now (: