Expose a SleepFuture::reset

code review
1

Code: Draft: sleepfuture-reset: Create new `SleepFuture` trait with `reset` method (!3287) · Merge requests · The Tor Project / Core / Arti · GitLab

Hi

I am contributing to the Arti project with the goal to become more fluent in Rust. So I tried the following issue: rtcompat: Expose a SleepFuture::reset (#2176) · Issues · The Tor Project / Core / Arti · GitLab.

In e.g maybenot_padding.rs:L903 you see we currently do:

// TODO circpad: Avoid rebuilding sleep future needlessly.  May require new APIs in
// tor-rtcompat.
self.sleep_future = runtime.sleep(t.saturating_duration_since(now)); // We create a new runtime each time we want to reset the expiration time.

The author of the issue provided to add a reset method in our tor-rtcompat API to expose e.g Tokio's Sleep in tokio::time - Rust reset so we can change the interval of the future without the need of creating a new one.

FYI, tor-rtcompat is a crate in Arti to be support compatibility between different runtimes like tokio, async-std or smol.

I made the following draft MR which already implements this for tokio, but I'd like some feedback and double-check my understanding: Draft: sleepfuture-reset: Create new `SleepFuture` trait with `reset` method (!3287) · Merge requests · The Tor Project / Core / Arti · GitLab.

Commit f1416858 - Create new SleepFuture trait with reset method

Previously SleepFuture was just a type type SleepFuture: Future<Output = ()> + Send + 'static; without any implementation. So I made a trait defining reset() method.

pub trait SleepFuture: Future<Output = ()> + Send + 'static {
    /// Reset the sleep future to expire at `instant`.
    ///
    /// With this method we do not have to create a new sleep future.
    fn reset(self: std::pin::Pin<&mut Self>, instant: Instant);
}

I added a few supertrait bounds to make sure that each type implementing SleepFuture also implements these:

  • Future<Output = ()>: Make sure any type implementing is also a Future.
  • Send: The implementing type must be able to be send safely across threads.
  • 'static: The implementing type may not have properties with a limited lifetime or borrowing data. This to make sure all the data from the implementing type can fully life over several threads etc.

I also made SleepProvider have the type type SleepFuture: SleepFuture; so each implementation can decide what implementation of SleepFuture to use (Tokio, async-std, smol,...)

Is this correct?

Commit 219bd7d8 - Update Tokio implementation with a new TokioSleep type to implement SleepFuture

Here I make the actual Tokio implementation of the new SleepFuture trait.

First I make a new type TokioSleep:

// Wrapper around a tokio sleep future.
// We use this wrapper so that we can implement SleepFuture on it.
pub struct TokioSleep(std::pin::Pin<Box<tokio_crate::time::Sleep>>);

It's actually just a wrapper around Tokio's sleep implementation which has the actual reset-method: Sleep in tokio::time - Rust

TokioSleep is not future, but our trait requires this due to the supertrait bounds. So I make it future:

// Make TokioSleep a Future by delegating to the inner future.
impl Future for TokioSleep {
    type Output = ();

    fn poll(
        self: std::pin::Pin<&mut Self>,
        cx: &mut std::task::Context<'_>,
    ) -> std::task::Poll<Self::Output> {
        self.get_mut().0.as_mut().poll(cx)
    }
}

This reset method just calls reset on the inner future (being Tokio):

// Implement SleepFuture by delegating to the inner future.
impl SleepFuture for TokioSleep {
    fn reset(self: std::pin::Pin<&mut Self>, instant: std::time::Instant) {
        self.get_mut()
            .0
            .as_mut()
            .reset(tokio_crate::time::Instant::from_std(instant));
    }
}

The method signature is uses type std::time::Instant for the instant parameter to be compatible with the trait definition. In the actual implementation I convert this to a tokio::time::Instant using from_std.

Pin is necessary because the future may contain internal references, and we want to prohibit moving this in data to avoid references to old/invalid locations in memory.

I updated the sleep method in SleepProvider to wrap the returned type in our new TokioSleep so the returned type can use reset.

Is this correct?

Commit fdad727f - Make DynSleepFuture forward the reset call to the inner SleepFuture

DynSleepFuture is a type which makes it possible to support different implementations of SleepFuture (Tokio, async-std, smol,...) at compile time. But when I would use DynSleepFuture in my code it would not implement SleepFuture resulting in my usages not being able to find the new reset method.

That's why I explicitly implemented it for DynSleepFuture.

// Make DynSleepFuture a SleepFuture explicitly.
impl SleepFuture for DynSleepFuture {
    fn reset(mut self: Pin<&mut Pin<Box<(dyn SleepFuture + 'static)>>>, instant: std::time::Instant) {
        let inner: Pin<&mut dyn SleepFuture> = self.as_mut();
        inner.reset(instant);
    }
}

This is actually just glue code to make DynSleepFuture forward the reset call to the actual SleepFuture from e.g Tokio.

Is this correct?

Commit ac3e33a7 - Update tor-rtmock to implement stubs for SleepFuture

tor-rtmock is used for testing. I implemented basic stubs for SleepFuture so the examples can keep executing.

Some feedback and learnings would be highly appreciated! Thanks in advance!

2

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.