feat: add downcast on `Sleep` trait by dswij · Pull Request #3125 · hyperium/hyper

dswij · 2023-01-23T17:54:48Z

This pr adds a downcast for the Sleep trait.

dswij · 2023-01-23T18:05:18Z

I'm not sure I understand #3027 completely, but I'm thinking of something like this.

Also, I noticed that there is no test for this. Am I missing something, or is this something we can add on?

sfackler · 2023-01-24T12:57:45Z

I think it'd be a bit cleaner to implement downcasting methods directly on Sleep without requiring implementors to deal with it:

mod private {
    pub struct PrivacyToken;
}

pub trait Sleep: Send + Sync + Future<Output = ()> {
    // PrivacyToken can't be named outside of this crate, so it prevents anyone from overriding this default
    // implementation in another crate. That allows us to trust it to be correct in the downcast methods below.
    #[doc(hidden)]
    fn __private_api_type_id(&self, _: private::PrivacyToken) -> TypeId {
        TypeId::of::<Self>()
    }
}

impl dyn Sleep {
    /// Returns `true` if the sleep's type is `T`.
    pub fn is<T>(&self) -> bool
    where
        T: Sleep,
    {
        self.__private_api_type_id(private::PrivacyToken) == TypeId::of::<T>()
    }

    /// Attempts to downcast the sleep to the type `T` if it has that type.
    pub fn downcast_ref<T>(&self) -> Option<&T>
    where
        T: Sleep,
    {
        if self.is::<T>() {
            unsafe { Some(&*(self as *const dyn Sleep as *const T)) }
        } else {
            None
        }
    }

    // downcast_pin as well similarly
}

dswij · 2023-01-24T16:23:35Z

I think it'd be a bit cleaner to implement downcasting methods directly on Sleep without requiring implementors to deal with it

That's a good point. We can probably make Sleep a derivable sealed trait instead.

Though, TypeId::of::<T> requires T: 'static, so we might want a workaround similar to this, if we wish to go that direction.

sfackler · 2023-01-25T13:12:17Z

Are there any relevant non-'static Sleep implementations?

dswij · 2023-01-29T19:47:51Z

Are there any relevant non-'static Sleep implementations?

I don't think so, but we shouldn't be too concerned about it now anyways imo. I have also added some docs to it

seanmonstar

I think this looks goot to me, @sfackler anything else you'd need?

seanmonstar · 2023-02-09T15:45:07Z

src/rt/mod.rs

+
+pub mod timer;
+
+pub use timer::*;


I'd suggest this be an explicit list, just to prevent any accidents now or in the future.

seanmonstar · 2023-02-09T15:45:30Z

src/rt/mod.rs

+//! If the `runtime` feature is disabled, the types in this module can be used
+//! to plug in other runtimes.
+
+pub mod timer;


Perhaps let's start with the module being private, we can always make it public later if it'd be helpful.

sfackler · 2023-02-10T01:56:53Z

src/rt/timer.rs

+/// A future returned by a `Timer`.
+pub trait Sleep: Send + Sync + Future<Output = ()> {
+    #[doc(hidden)]
+    /// This method is private and should not be implemented by downstream crate


can not be implemented, not should not be implemented.

sfackler · 2023-02-10T01:58:11Z

src/rt/timer.rs

+//!         if sleep.downcast_ref::<TokioSleep>().is_some() {
+//!             *sleep = self.sleep_until(new_deadline);
+//!         }


This doesn't make much sense to me. The whole point of downcasting is to not have to create a new timer.

Think I understood the issue better now. I updated the example code with an implementation of tokio::time::Sleep::reset

sfackler · 2023-02-10T02:00:48Z

src/rt/timer.rs

+    /// Downcast the Sleep object to its original type
+    pub fn downcast_ref<T>(&self) -> Option<&T>
+    where
+        T: Sleep + 'static,
+    {
+        if self.is::<T>() {
+            unsafe { Some(&*(self as *const dyn Sleep as *const T)) }
+        } else {
+            None
+        }
+    }
+
+    /// Similar to `downcast_ref` but returns a mutable version instead
+    pub fn downcast_mut<T>(&mut self) -> Option<&mut T>
+    where
+        T: Sleep + 'static,
+    {
+        if self.is::<T>() {
+            unsafe { Some(&mut *(self as *mut dyn Sleep as *mut T)) }
+        } else {
+            None
+        }
+    }
+}


I don't think these two downcast methods are going to be too useful, since reset is working with a &mut Pin<Box<dyn Sleep>>. I think we'd probably need a pub fn downcast_pin_mut<T>(self: Pin<&mut Self>) -> Option<Pin<&mut T>> probably?

sfackler · 2023-02-10T02:01:56Z

I think it'd be good to update the test TokioTimer to downcast in its reset implementation to test that all of this works in practice: https://github.com/hyperium/hyper/blob/master/benches/support/tokiort.rs#L29-L44

seanmonstar · 2023-02-23T14:53:20Z

Hey @dswij, just wanted to check if you think you'll have time to update this. No worries if you can't, just checking to reduce duplicate work. :)

dswij · 2023-02-24T16:02:37Z

Hey @dswij, just wanted to check if you think you'll have time to update this. No worries if you can't, just checking to reduce duplicate work. :)

Sorry for the delay, I was on a break a while ago and I haven't caught up to my backlog yet.

I'll try to take a look at this PR on the weekend 🙂

This commit adds `as_any` downcast method for the `Sleep` trait. BREAKING CHANGE: `Sleep` trait now needs `as_any` implementation

This commit allows downcasting pinned `Sleep` object to support implementations of `Timer::reset`. One example where this is useful is when using `TokioSleep`, i.e. `Sleep` provided by tokio.

dswij · 2023-03-22T15:41:05Z

@seanmonstar @sfackler sorry for the delay, turned out it took a while for me to get back to this.

Would you guys able to help give another round of review?

dswij · 2023-03-30T18:43:04Z

@seanmonstar sorry to nag, but is there anything I can do to move this forward?

This commit allows downcasting pinned `Sleep` object to support implementations of `Timer::reset`. One example where this is useful is when using `TokioSleep`, i.e. `Sleep` provided by tokio. Closes hyperium#3027

This commit allows downcasting pinned `Sleep` object to support implementations of `Timer::reset`. One example where this is useful is when using `TokioSleep`, i.e. `Sleep` provided by tokio. Closes hyperium#3027 Signed-off-by: Sven Pfennig <s.pfennig@reply.de>

dswij force-pushed the issue-3027 branch from ca96545 to 2ee82a7 Compare January 23, 2023 18:04

seanmonstar requested a review from sfackler January 23, 2023 20:04

dswij force-pushed the issue-3027 branch 2 times, most recently from fc9e8f3 to b7d56c9 Compare January 29, 2023 15:30

seanmonstar reviewed Feb 9, 2023

View reviewed changes

sfackler reviewed Feb 10, 2023

View reviewed changes

dswij force-pushed the issue-3027 branch 2 times, most recently from 4dc245c to 6858fab Compare March 13, 2023 17:24

dswij added 4 commits March 22, 2023 22:41

feat: add downcast on Sleep trait

a6e2355

This commit adds `as_any` downcast method for the `Sleep` trait. BREAKING CHANGE: `Sleep` trait now needs `as_any` implementation

feat: make rt::timer private

2e9fdb6

feat: update timer description

424e396

tests: update TokioTimer to use downcast methods

50c527f

dswij force-pushed the issue-3027 branch from 806a943 to 450e2a9 Compare March 22, 2023 14:42

dswij requested review from seanmonstar and sfackler and removed request for seanmonstar and sfackler March 22, 2023 15:36

feat: allow downcasting pinned Sleep

8d0e134

This commit allows downcasting pinned `Sleep` object to support implementations of `Timer::reset`. One example where this is useful is when using `TokioSleep`, i.e. `Sleep` provided by tokio.

dswij force-pushed the issue-3027 branch from 450e2a9 to 8d0e134 Compare March 22, 2023 15:37

seanmonstar requested a review from sfackler March 30, 2023 18:44

Merge branch 'master' into issue-3027

b2b64eb

seanmonstar approved these changes Jul 6, 2023

View reviewed changes

seanmonstar merged commit d92d391 into hyperium:master Jul 6, 2023

dswij deleted the issue-3027 branch July 6, 2023 14:06

Uh oh!

Conversation

dswij commented Jan 23, 2023

Uh oh!

dswij commented Jan 23, 2023

Uh oh!

sfackler commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dswij commented Jan 24, 2023

Uh oh!

sfackler commented Jan 25, 2023

Uh oh!

dswij commented Jan 29, 2023

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfackler commented Feb 10, 2023

Uh oh!

seanmonstar commented Feb 23, 2023

Uh oh!

dswij commented Feb 24, 2023

Uh oh!

dswij commented Mar 22, 2023

Uh oh!

dswij commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sfackler commented Jan 24, 2023 •

edited

Loading