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.

[seanmonstar]

Hey! Looks interesting! It does seem to be backwards compatible, definitely!

While I might have a little personal doubt of provided instant type, I'd be happy to see something make that doubt go away...

I think I'd be most interested if there was a way to allow this idea to be explored and tried and vetted, before making something a bit more complicated like this stable in hyper's v1 public API. Possibly via some hyper_unstable_timer flag ( or if there was a way to hack something in without hyper knowing, that'd be even better, but probably not).

[xetqL]

Hi, thanks for the feedback.

As it seems that the idea makes some sense at least, I will write a PR for it. It definitely needs to be polished and protecting it behind a feature flag at first seems a good idea.

I've already worked on some similar stuff/techniques for some network protocol librairies that needed the time from outside of the crate; so i am pretty confident it works.

Let me work on that and share the result

[arielb1]

You could have this:

trait Timer {
    fn now(&self) -> std::time::Instant {
        // default
        std::time::Instant::now()
    }
}

edit: a previous version had that on Sleep instead of Timer

Since std::time::Instant and tokio::time::Instant are interconvertible

The main problem with having a "variable-sized" instant type is that a bunch of Hyper internal logic will have to be generic over it.

For @xetqL - the annoying thing is that there is no (safe) way to create an instant without calling now at least once (well, you can take a random bunch of bytes and transmute it, but that is not good). After you have an instant, you can do operations without more system calls, but that does not help you get the first instant.

[seanmonstar]

Oh, I think I like that a lot. Though, on the Timer trait, not on Sleep.

Is there any other problems with that kind of change that should be discussed? If not, I'd prefer a PR with just the new method and docs, and a follow-up PR then using it within hyper.

[xetqL]

@arielb1 well, std::time::instant or tokio one makes a lot of assumption on the system side (which clock is used for instance) and maybe you want a single centralized clock (do not repeat the syscall).

The problem I have with all of this is that to create such Instant you have to do a syscall (you can not mock an instant) and you can not control that syscall (which clock is used).

[seanmonstar]

Some sort of Instant::ZERO or something would be handy in this case, relevant issue: https://github.com/rust-lang/rust/issues/40910

[arielb1]

cc my PR - #3965

[arielb1]

We could make Timer work with this:

enum Instant {
    Std(std::time::Instant),
    Offset(std::time::Duration),
}

But that feels like over-engineering to me. In any case we could always do that in a later version.

The problem I have with all of this is that to create such Instant you have to do a syscall (you can not mock an instant) and you can not control that syscall (which clock is used).

If you just use an Instant as a zero point, you don't care about which clock is used.

If you don't care about portability, you could just transmute (or ptr::read) a large amount of zeroes into an Instant

[xetqL]

@arielb1 it's incorrect, some clocks take into account sleep time some other does not (boottime vs monotonic https://man7.org/linux/man-pages/man3/clock_gettime.3.html). It's not only the epoch that is important in some cases.

[arielb1]

If you use Instant as fake, you don't care. Either call the syscall once to get a base point, or transmute to get a base point, then do duration arithmetic.

[xetqL]

That's a very small subnet of use cases unfortunately. I believe that hyper should be as portable, fast, and accurate as possible

[arielb1]

That's a very small subnet of use cases unfortunately. I believe that hyper should be as portable, fast, and accurate as possible

Most uses of Hyper can use the std Instant (and std now).

Users that want their own timing mechanism, can get an Instant base point once (this is annoying, but not that annoying), then do duration arithmetic based on that. You can use whatever clock you want with duration arithmetic.

[xetqL]

It's a design decision to say that hyper won't build no_std, I believe that it's not very reasonable for a pure http library that is able to delegate most of the syscalls.

I could also argue that it's not that a big change to use a custom instant type (its annoying but not that annoying) nor it creates a lot of work.

Finally, to know time difference one need to get the current time point, which puts us back to our starting point. If I am building no_std, how do I construct a damn std::time::Instant?

[arielb1]

In any case, adding a Timer::now is backwards-compatible with adding an associated type way of creating instants.

The real reason having a fixed type is nice is because Hyper currently uses a trait object for timers, and trait objects don't work with associated types.

Tho, there is probably no problem in having Timer be a trait alias to AgnosticTimer<Instant = std::time::Instant> (where e.g. the pool can't use an agnostic timer).

[seanmonstar]

Thanks for the discussion, this was helpful! This what I'm noticing:

• Providing an overridable fn now() -> Instant is a minimal addition, and would allow integrations with pausable clocks, like Tokio's test utils.
• However, it still requires at least one syscall-ish to create at least one, since the only constructor is Instant::now().
  • If there was a new constructor to Instant which doesn't require the system, that would solve the other use case.
  • In my opinion, it seems better for crates in general to expect the type from libstd (or libcore if Instant::ZERO allows moving it), than for every crate to define it's own InstantLike type.
• It could be possible to switch to some AgnosticTimer<Instant = Something> (name maybe Clock<Instant = ...>?), but that's a more extensive change, and needs to be tested out if it would fully work.

Thus, I'm leaning towards we add fn now() -> Instant to the Timer trait to start. And I'd recommend pushing on https://github.com/rust-lang/rust/issues/40910 for other use cases.

Does that seem reasonable?

[arielb1]

That looks right to me.

[arielb1]

Any update on this?

[seanmonstar]

I was just giving some wait time in case of any other comments. But we can move forward your PR 👍