feat(client, server) make timers generic

[Robert-Cunningham]

Hi Sean,

I'm working through the timer series of issues (#2847, #2846, #2848, #2857) right now. This PR isn't (near) done, but I wanted to do a midway check-in and see if you agree with the design decisions, or have any feedback that I can integrate now, relatively early on in the process.

In particular, I'd appreciate any broad design feedback you have on rt.rs or tim.rs.

Otherwise I'll continue moving forwards with this series of issues!

Thanks for your work on hyper!

[Robert-Cunningham]

One note: pause, advance and resume from tokio::time control only tokio::time::Instant, not std::time::Instant. If we want to retain the ability to use functions like tokio::time::pause, we need to somehow make all the Instant objects tokio::time::Instant (or potentially any other Instant-ish type for a different library) when the user uses Tokio timers.

If we go that route, I believe we'd need to add functions like Instant::now() to the Timer trait and then and pass Timers around to anywhere we need to call Instant::now().

The alternative is just to use std::time::Instant everywhere and choose not to support functions like tokio::time::pause().

I suspect allowing changes to our Instant type is not worth the additional complexity, but let me know if you think otherwise.

[seanmonstar]

Yea I don't want to depend directly on tokio::time::Instant. It might make sense to eventually allow the trait Timer to tell us what the time is, to allow mocking... But I'd probably start with having whatever tests be commented out with a TODO to figure them out later.

[Robert-Cunningham]

Alright, I'm now working through the last major hiccup in this PR--would appreciate some guidance here if possible (potentially from @seanmonstar ?).

I have a trait Timer mirroring tokio::time::* with functions like sleep and reset. The last function I need is in Timer is timeout<T>, but I can't include it directly without making Timer object-unsafe. (If Timer becomes object-unsafe, we have to pass it as a generic all over the code base, which I tried before and quickly becomes disgusting.)

I'm trying to cook up a function signature which will allow me to include timeout<T> in the Timer trait, something like fn timeout(&self, duration: Duration, future: Box<dyn Future<Output = Box<dyn Any>>>). Then inside the function I need to do something like tokio::time::timeout(duration, *future), but this doesn't work because future isn't Sized (makes sense).

Do you have any idea how I can include or wrap something similar to tokio::time::timeout<T> in an object-safe Timer trait?

[ibraheemdev]

@Robert-Cunningham you can write your own timeout function that wraps sleep:

pub fn timeout<F>(duration: Duration, future: F) -> Timeout<F>
where
    F: Future,
{
    Timeout {
        future,
        sleep: sleep(duration),
    }
}


pin_project_lite::pin_project! {
    pub struct Timeout<F> {
        #[pin]
        future: F,
        sleep: Box<dyn Sleep>
    }
}

impl<F> Future for Timeout<F>
where
    F: Future,
{
    type Output = Result<F::Output, TimedOut>;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        let mut this = self.project();

        if let Poll::Ready(v) = this.future.poll(cx) {
            return Poll::Ready(Ok(v));
        }

        match Pin::new(&mut this.sleep).poll(cx) {
            Poll::Ready(()) => Poll::Ready(Err(TimedOut)),
            Poll::Pending => Poll::Pending,
        }
    }
}

[Robert-Cunningham]

Thanks a bunch @ibraheemdev; your answer was instructive for me. With that, I think this PR is ready for review (from @seanmonstar, I think?). Please let me know if I need to do anything else before review; this is my first contribution to hyper.

Once this PR is merged, I'll open a PR that adds the same TokioTimer to hyper-util.

[seanmonstar]

Thanks so much for working on this! I'm at RustConf today, but I'll take a look beginning of next week <3

[seanmonstar]

I'm really impressed with this PR, it looks very solid! ❤️

I'm sorry for having taken a while to be able to review it, there were a lot of changes touching the same files and juggling them all in my head was too much. But most of the other changes are done, I can focus on the timer changes now. To that end, there's some code in here that can be removed (since Client and it's pool are goners now). If it's too annoying to do, I can clean it up, just let me know. :)

[seanmonstar]

We could chose not to implement the trait on the type directly, and instead just have inherent methods. There's also a question of whether it should panic like this, or if we should check "if timer isn't set, don't do the time-related thing".

[Robert-Cunningham]

On the second point, my original reasoning was that if you've tried set up something like header reading timeouts, but we can't do it because you didn't give us a timer, we should fail loudly (panic). The failure mode otherwise--not doing the timeouts--seems subtle and likely only to be discovered when it causes a problem for the user in the future.

On the first point, I'm not sure I understand your suggestion fully. You're saying that instead of implementing Timer on Time, we could add these as inherent methods of Timer? Would you mind elaborating a bit more?

[seanmonstar]

Do you feel including the generic provides much value? Since the common::time::Time enum already holds an arced trait object, holding onto that could eliminate the need for the timer.

(The generic for the executor is different, it's propagating the Sendiness of the future to the task that is spawned. I don't believe there's anything a timer needs to propagate there.)

[seanmonstar]

Do we need these methods? They do seem convenient, but if hyper can live without them, that may be easier for different timer implementations that might not keep that information around. Or that might not be easily reset.

@dhobsd would these methods be hard to implement in Fuchsia land?

[dhobsd]

Thanks for remembering us and reaching out! (Sorry for delay!)

Fuchsia's async executor uses deadlines for its time, so that's at least good. Two points to bring up:

1. reset in tokio says "This function can be called both before and after the future has completed." In fuchsia_async's Timer, this isn't true -- you can only reset a timer that's already fired. I'm also not sure what it means to reset a timer that hasn't fired because it's hard to guarantee you won't observe the signal. Does hyper reset timers that haven't fired?
2. is_elapsed says that it returns true when the time has elapsed, but is this the same as saying the timer has already fired?

I guess a third question: will hyper rely on these divergent behaviors? If not, perhaps we can make the documentation stricter in hyper's trait?

[Robert-Cunningham]

Ah, thanks for your feedback--I appreciate the detail. I'm headed to burning man until September 6, but I'm going to try to get this over the goal line before I arrive today.

If I don't manage to finish before I lose cell service, please feel free to make any changes as you see fit and merge--I don't want to stand in the way of progress on 1.0!

[seanmonstar]

Thanks so much for taking it this far! I've opened #2974 that rebases this, and I may tweak a little more, and then merge.