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

Timer::reset can't soundly have an optimized implementation #3027

Closed
sfackler opened this issue Oct 27, 2022 · 2 comments · Fixed by #3125
Closed

Timer::reset can't soundly have an optimized implementation #3027

sfackler opened this issue Oct 27, 2022 · 2 comments · Fixed by #3125
Labels
A-rt Area: runtime traits/utils C-performance Category: performance. This is making existing behavior go faster. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Milestone

Comments

@sfackler
Copy link
Contributor

In 1.0.0-rc.1, the new Timer trait has a method to reset an existing Sleep to a new deadline. The default implementation simply creates a new Sleep and writes it over the old one, but presumably this method exists to it can be overridden with a better implementation when working with a timer like Tokio's that can have its deadline dynamically adjusted.

However, since the Sleep is provided as a dyn Sleep and Sleep has no downcasting APIs, there is no safe way to confirm that the Sleep instance is one created by this Timer implementation. That effectively means that the current default implementation is the only sound way of implementing the method.

@sfackler sfackler added the C-bug Category: bug. Something is wrong. This is bad! label Oct 27, 2022
@seanmonstar seanmonstar added the B-breaking-change Blocked: this is an "API breaking change". label Nov 8, 2022
@seanmonstar seanmonstar added this to the 1.0 RC2 milestone Nov 8, 2022
@seanmonstar
Copy link
Member

Sigh, yea, I forgot about this. And Rust still hasn't made this any easier; we can't just say trait Sleep: Any. So, let's see, I think these are the options:

  • Copy the methods from Any into Sleep, which allows checking type_id and then calling downcast_mut. To make it safe, it probably needs to implement those methods in a blanket impl. Do we just impl<F: Future> Sleep for F {}?
  • Maybe we can move the method to trait Sleep { fn reset(self: Pin<&mut Self>, timer: &dyn Timer) {}? That might just require having downcast methods on Timer instead...
  • Ugh.

@sfackler
Copy link
Contributor Author

Sleep: Any won't handle it, but you can manually add downcasting to the Sleep trait.

@seanmonstar seanmonstar modified the milestones: 1.0 RC2, 1.0 RC3 Dec 29, 2022
@seanmonstar seanmonstar added E-easy Effort: easy. A task that would be a great starting point for a new contributor. A-rt Area: runtime traits/utils C-performance Category: performance. This is making existing behavior go faster. and removed B-breaking-change Blocked: this is an "API breaking change". C-bug Category: bug. Something is wrong. This is bad! labels Jan 5, 2023
@seanmonstar seanmonstar modified the milestones: 1.0 RC3, 1.0 RC4 Feb 24, 2023
seanmonstar pushed a commit that referenced this issue Jul 6, 2023
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 #3027
mmastrac added a commit to denoland/deno that referenced this issue Jul 31, 2023
Includes a lightly-modified version of hyper-util's `TokioIo` utility. 

Hyper changes:

v1.0.0-rc.4 (2023-07-10)
Bug Fixes

    http1:
http1 server graceful shutdown fix (#3261)
([f4b51300](hyperium/hyper@f4b5130))
send error on Incoming body when connection errors (#3256)
([52f19259](hyperium/hyper@52f1925),
closes hyperium/hyper#3253)
properly end chunked bodies when it was known to be empty (#3254)
([fec64cf0](hyperium/hyper@fec64cf),
closes hyperium/hyper#3252)

Features

client: Make clients able to use non-Send executor (#3184)
([d977f209](hyperium/hyper@d977f20),
closes hyperium/hyper#3017)
    rt:
replace IO traits with hyper::rt ones (#3230)
([f9f65b7a](hyperium/hyper@f9f65b7),
closes hyperium/hyper#3110)
add downcast on Sleep trait (#3125)
([d92d3917](hyperium/hyper@d92d391),
closes hyperium/hyper#3027)
service: change Service::call to take &self (#3223)
([d894439e](hyperium/hyper@d894439),
closes hyperium/hyper#3040)

Breaking Changes

Any IO transport type provided must not implement hyper::rt::{Read,
Write} instead of tokio::io traits. You can grab a helper type from
hyper-util to wrap Tokio types, or implement the traits yourself, if
it's a custom type.
([f9f65b7a](hyperium/hyper@f9f65b7))
client::conn::http2 types now use another generic for an Executor. Code
that names Connection needs to include the additional generic parameter.
([d977f209](hyperium/hyper@d977f20))
The Service::call function no longer takes a mutable reference to self.
The FnMut trait bound on the service::util::service_fn function and the
trait bound on the impl for the ServiceFn struct were changed from FnMut
to Fn.
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this issue Jan 12, 2024
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
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this issue Jan 16, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rt Area: runtime traits/utils C-performance Category: performance. This is making existing behavior go faster. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants