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

feat(rt): add Timer trait #2974

Merged
merged 1 commit into from
Sep 2, 2022
Merged

feat(rt): add Timer trait #2974

merged 1 commit into from
Sep 2, 2022

Conversation

seanmonstar
Copy link
Member

This adds a hyper::rt::Timer trait, and it is used in connection
builders to configure a custom timer source for timeouts.

Closes #2846
Closes #2847
Closes #2848

Continues #2904

@seanmonstar seanmonstar force-pushed the pr/2904 branch 3 times, most recently from a09a1e5 to 0438359 Compare August 31, 2022 23:46
@seanmonstar seanmonstar force-pushed the pr/2904 branch 2 times, most recently from 8d69380 to d2889c4 Compare September 1, 2022 00:07
src/rt.rs Outdated
fn deadline(&self) -> Instant;

/// An analogue of tokio::time::Sleep::reset.
fn reset(self: Pin<&mut Self>, deadline: Instant);
Copy link
Member Author

Choose a reason for hiding this comment

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

Continuing the conversation, where @dhobsd said:

Thanks for remembering us and reaching out! (Sorry for delay!)

Fuchsia's async executor uses deadlines for its time, so that's at least good. Two points to bring up:

  • reset in tokio says "This function can be called both before and after the future has completed." In fuchsia_async's Timer, this isn't true -- you can only reset a timer that's already fired. I'm also not sure what it means to reset a timer that hasn't fired because it's hard to guarantee you won't observe the signal. Does hyper reset timers that haven't fired?
  • is_elapsed says that it returns true when the time has elapsed, but is this the same as saying the timer has already fired?

I guess a third question: will hyper rely on these divergent behaviors? If not, perhaps we can make the documentation stricter in hyper's trait?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I looked around how we use these methods, and here's what I've found:

  • reset is used for the HTTP/1 header read timeout, so that successive reads simply move the timer further out, instead of allocating a new one.
  • Doesn't look like we use the other methods, so I've been happy to toss them.

The reset method seems to just be to allow for optimizations, but we could simply provide a default implementation of the method somehow... Basically, if it didn't exist, we'd just call timer.sleep(next_dur) and replace the old one. Hm, that might mean the method needs to be on Timer instead, since we can't make a default implementation that creates a new sleep without the Timer. Maybe then a timer.reset(sleep, next_dur).

We even could just leave it off entirely to start with, since adding a method with a default implementation is fine. It would just mean potentially unnecessary allocations when using Tokio.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've done that: moved the reset method to Timer, and it has a default implementation that just allocates a new sleep.

@seanmonstar seanmonstar force-pushed the pr/2904 branch 10 times, most recently from 60894f1 to 746e60d Compare September 2, 2022 00:36
This adds a `hyper::rt::Timer` trait, and it is used in connection
builders to configure a custom timer source for timeouts.
@seanmonstar seanmonstar merged commit fae97ce into master Sep 2, 2022
@seanmonstar seanmonstar deleted the pr/2904 branch September 2, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants