Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(rt): Clean up Timer trait #3037

Merged
merged 2 commits into from Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 16 additions & 12 deletions benches/support/tokiort.rs
Expand Up @@ -8,6 +8,7 @@ use std::{

use futures_util::Future;
use hyper::rt::{Sleep, Timer};
use pin_project_lite::pin_project;

#[derive(Clone)]
/// An Executor that uses the tokio runtime.
Expand All @@ -29,16 +30,16 @@ where
pub struct TokioTimer;

impl Timer for TokioTimer {
fn sleep(&self, duration: Duration) -> Box<dyn Sleep + Unpin> {
let s = tokio::time::sleep(duration);
let hs = TokioSleep { inner: Box::pin(s) };
return Box::new(hs);
fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>> {
Box::pin(TokioSleep {
inner: tokio::time::sleep(duration),
})
}

fn sleep_until(&self, deadline: Instant) -> Box<dyn Sleep + Unpin> {
return Box::new(TokioSleep {
inner: Box::pin(tokio::time::sleep_until(deadline.into())),
});
fn sleep_until(&self, deadline: Instant) -> Pin<Box<dyn Sleep>> {
Box::pin(TokioSleep {
inner: tokio::time::sleep_until(deadline.into()),
})
}
}

Expand All @@ -59,15 +60,18 @@ where

// Use TokioSleep to get tokio::time::Sleep to implement Unpin.
// see https://docs.rs/tokio/latest/tokio/time/struct.Sleep.html
pub(crate) struct TokioSleep {
pub(crate) inner: Pin<Box<tokio::time::Sleep>>,
pin_project! {
pub(crate) struct TokioSleep {
#[pin]
pub(crate) inner: tokio::time::Sleep,
}
}

impl Future for TokioSleep {
type Output = ();

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
self.inner.as_mut().poll(cx)
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
self.project().inner.poll(cx)
}
}

Expand Down
7 changes: 0 additions & 7 deletions src/common/mod.rs
Expand Up @@ -28,10 +28,3 @@ cfg_proto! {
pub(crate) use std::marker::Unpin;
}
pub(crate) use std::{future::Future, pin::Pin};

pub(crate) fn into_pin<T: ?Sized>(boxed: Box<T>) -> Pin<Box<T>> {
// It's not possible to move or replace the insides of a `Pin<Box<T>>`
// when `T: !Unpin`, so it's safe to pin it directly without any
// additional requirements.
unsafe { Pin::new_unchecked(boxed) }
}
4 changes: 2 additions & 2 deletions src/common/time.rs
Expand Up @@ -55,7 +55,7 @@ impl<F> Future for HyperTimeout<F> where F: Future {
*/

impl Time {
pub(crate) fn sleep(&self, duration: Duration) -> Box<dyn Sleep + Unpin> {
pub(crate) fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>> {
match *self {
Time::Empty => {
panic!("You must supply a timer.")
Expand All @@ -64,7 +64,7 @@ impl Time {
}
}

pub(crate) fn sleep_until(&self, deadline: Instant) -> Box<dyn Sleep + Unpin> {
pub(crate) fn sleep_until(&self, deadline: Instant) -> Pin<Box<dyn Sleep>> {
match *self {
Time::Empty => {
panic!("You must supply a timer.")
Expand Down
3 changes: 1 addition & 2 deletions src/proto/h1/role.rs
Expand Up @@ -87,8 +87,7 @@ where
}
None => {
debug!("setting h1 header read timeout timer");
*ctx.h1_header_read_timeout_fut =
Some(crate::common::into_pin(ctx.timer.sleep_until(deadline)));
*ctx.h1_header_read_timeout_fut = Some(ctx.timer.sleep_until(deadline));
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/proto/h2/ping.rs
Expand Up @@ -61,7 +61,7 @@ pub(super) fn channel(ping_pong: PingPong, config: Config, __timer: Time) -> (Re
interval,
timeout: config.keep_alive_timeout,
while_idle: config.keep_alive_while_idle,
sleep: crate::common::into_pin(__timer.sleep(interval)),
sleep: __timer.sleep(interval),
state: KeepAliveState::Init,
timer: __timer,
});
Expand Down
8 changes: 4 additions & 4 deletions src/rt.rs
Expand Up @@ -20,16 +20,16 @@ pub trait Executor<Fut> {
/// A timer which provides timer-like functions.
pub trait Timer {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: Would it be good to have the resulting futures as an associated type rather than as a boxed future? Both tokio and smol have well-defined "sleep" types instead of async methods, so we may be able to avoid an allocation here by not boxing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require parameterizing the ~entire API over the timer trait which I'm not sure is desired.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a workaround, you could just do:

struct BoxedTimer {
    inner: Box<dyn Timer<Sleep = Pin<Box<dyn Future>>>>>
}

But this might not be idiomatic, so I agree with your assessment.

/// Return a future that resolves in `duration` time.
fn sleep(&self, duration: Duration) -> Box<dyn Sleep + Unpin>;
fn sleep(&self, duration: Duration) -> Pin<Box<dyn Sleep>>;

/// Return a future that resolves at `deadline`.
fn sleep_until(&self, deadline: Instant) -> Box<dyn Sleep + Unpin>;
fn sleep_until(&self, deadline: Instant) -> Pin<Box<dyn Sleep>>;

/// Reset a future to resolve at `new_deadline` instead.
fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: Instant) {
*sleep = crate::common::into_pin(self.sleep_until(new_deadline));
*sleep = self.sleep_until(new_deadline);
}
}

/// A future returned by a `Timer`.
pub trait Sleep: Send + Sync + Unpin + Future<Output = ()> {}
pub trait Sleep: Send + Sync + Future<Output = ()> {}