Make hyper::rt::Timer instant agnostic

[xetqL]

Hi, first of all thanks for contributing and maintaining this super nice library. This work is greatly appreciated.

I would like to open a discussion about a potential improvement to the library.

I work in a project where we have a clear separation of pure and non-pure crates. This allows for reuse of code across platform for purely logic code. Hyper is one of our core components.

One issue we are encountering is regarding timers. Indeed, this trait requires the caller to have access to std::time::Instant for sleep_until, which is not something we can guarantee for various reasons I am not able to describe here. The sleep_until method seems to be used internally for HTTP/2 keep-alives ultimately generating system calls in places where we would like to be syscall free.

Ideally, hyper should be able to delegate side-effects, like it does for the sockets operations - the Read/Write trait do not expose a dependency to a structure that forces the caller to do a system call like std::time::Instant does.

My simple proposal is to make the trait instant agnostic. I think that there are two important aspect here:

1. The Timer trait allows to define the instant type to use
2. sleep_until depends on Self::Instant
3. Hyper requires a component implementing a trait abstracting how to get the current time via a now() method.

A very rudimentary proposal:

trait Timer {
  type Instant; 
  fn sleep_until(&self, deadline: Self::Instant) -> Pin<Box<dyn Sleep>>; 
  // skipping sleep and reset ... 
}

trait TimeFactory {
  type Instant; 
  fn now(&self) -> Self::Instant; 
  fn epoch(&self) -> Self::Instant; 
}

With the proposed approach, hyper won't have to rely on system calls to get an Instant if the calling code is not able to. In addition, the time could be mocked in test allowing a finer control and a faster test suite.

Thanks for your time

[seanmonstar]

Hey there! 👋

Thanks for the thoughtful write‑up and for sharing your use case. The change you’re suggesting would be a breaking change to the Timer trait. And I don't have any current plans to work on a hyper v2.

Possibly there's an alternative that exists that could both support your case and not break the existing API, if you want to explore that way.

[xetqL]

@seanmonstar thank you for your answer. Make sense if you are not planning a major bump then my proposal won't fit.

What is this possible alternative you are talking about? I'd be glad to know more about it.
Thanks again!

[seanmonstar]

I don't know the alternative, I meant to say maybe one exists if you want to try to find one. 🤓

[xetqL]

Ah! Sorry 😀 misunderstanding, unfortunately as long as one construct an Instant then it relies on the system. I don't see how we could avoid it unfortunately. We will probably rely on forking hyper.

Thank a lot Sean, I wish you all the best 👍 if we ever find a solution we will try tu upstream it.

[seanmonstar]

If you never configure any of the timeouts, it should never try to create an a sleep, right?

[xetqL]

We do rely on h2 keep-alives

[xetqL]

But yeah, in case one do not need timeouts hyper don't schedule it.
One way would have been to remove the sleep_until from hyper and only rely on sleep. That would be fine, but it means that there is no safeguard that sleep_until isn't used later on.

[xetqL]

Hey @seanmonstar, I am re-opening this after thinking about it a bit more...
What do you think about this backward compatible compatible proposal?

use std::{pin::Pin, sync::Arc, time::Duration};

trait Sleep {} // stub for the sake of shortness

/// Abstraction over how to get the current time
pub trait InstantProvider {
    type Instant;
    fn now() -> Self::Instant;
}

/// A timer that is agnostic over the current instant type
pub trait AgnosticTimer {
    type Instant;
    // Required methods
    fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>>;
    fn sleep_until(&self, deadline: Self::Instant) -> Pin<Box<dyn Sleep>>;

    // Provided method
    fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: Self::Instant);
}

/// The current Hyper timer trait
pub trait Timer {
    // Required methods
    fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>>;
    fn sleep_until(&self, deadline: std::time::Instant) -> Pin<Box<dyn Sleep>>;

    // Provided method
    fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: std::time::Instant);
}

/// The std instant provider, basically delegating to `std::time::Instant::now`
struct StdInstantProvider;

impl InstantProvider for StdInstantProvider {
    type Instant = std::time::Instant;

    fn now() -> Self::Instant {
        Self::Instant::now()
    }
}

/// A compatibility layer between [`AgnosticTimer`] and [`Timer`].
struct AgnosticTimerCompat<T>(T);
impl<T: Timer> AgnosticTimer for AgnosticTimerCompat<T> {
    type Instant = std::time::Instant;

    fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>> {
        self.0.sleep(duration)
    }

    fn sleep_until(&self, deadline: Self::Instant) -> Pin<Box<dyn Sleep>> {
        self.0.sleep_until(deadline)
    }
    
    fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: Self::Instant) {
        todo!()
    }
}

/// The new hyper internal time type
struct TimeSystem<IP: InstantProvider> {
    pub instant_provider: IP,
    pub timer:  Arc<dyn AgnosticTimer<Instant = IP::Instant>>
}

/// Blanket for all currently implemented timer (for backward compatibility)
impl<T: Timer + 'static> From<T> for TimeSystem<StdInstantProvider> {
    fn from(value: T) -> Self {
        TimeSystem { instant_provider: StdInstantProvider, timer: Arc::new(AgnosticTimerCompat(value)) }
    }
}

/// The constructor: this could be a bit more sophisticated in the end
impl<T: AgnosticTimer + 'static, IP: InstantProvider> From<(T, IP)> for TimeSystem<IP> where T: AgnosticTimer<Instant = IP::Instant> {
    fn from((timer, instant_provider): (T, IP)) -> Self {
        TimeSystem { instant_provider, timer: Arc::new(timer) }
    }
}

// --- example ---
struct Builder;
impl Builder {
    fn with_timer<T: InstantProvider>(&mut self, time_system: impl Into<TimeSystem<T>>) {
        todo!()
    }
}

fn main() {
    struct MyTimer; 
    impl Timer for MyTimer {
        fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>> {
            todo!()
        }
    
        fn sleep_until(&self, deadline: std::time::Instant) -> Pin<Box<dyn Sleep>> {
            todo!()
        }
    
        fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: std::time::Instant) {
            todo!()
        }
    }

    struct MyInstantProvider;
    struct MyInstant;
    impl InstantProvider for MyInstantProvider {
        type Instant = MyInstant;
    
        fn now() -> Self::Instant {
            todo!()
        }
    }

    struct MyNewTimer;
    impl AgnosticTimer for MyNewTimer {
        type Instant = MyInstant;
    
        fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>> {
            todo!()
        }
    
        fn sleep_until(&self, deadline: Self::Instant) -> Pin<Box<dyn Sleep>> {
            todo!()
        }
    
        fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: Self::Instant) {
            todo!()
        }
    }

    Builder.with_timer(MyTimer);
    Builder.with_timer((MyNewTimer, MyInstantProvider));
}

if you think this goes in the right direction, allow me to send a PR for it. Of course, this needs to be polished etc.

All the best

[xetqL]

Sorry to ping @seanmonstar but I have some free time I could use to work on it at the moment hence I'd like to know if the overall idea looks fine to you.