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

time: allow users to specify Interval behavior when delayed #3721

Merged
merged 18 commits into from
Jun 16, 2021

Conversation

eb-64-64
Copy link
Contributor

@eb-64-64 eb-64-64 commented Apr 23, 2021

Motivation

Occasionally, tokio::time::Interval may be delayed from ticking, usually because tick isn't being called often enough (like if a task is .awaiting something else). By default, Interval continues to schedule ticks normally, which causes a burst of ticks until it is "caught up" in time where it should be. This behavior is not always desired, however.

Solution

This PR adds the MissedTickBehavior enum to the time module to allow users to specify what behavior they'd like Interval to exhibit when ticks are missed.

Closes #3574

Occasionally, `tokio::time::Interval` may be delayed from ticking by a
blocked executor. By default, `Interval` continues to schedule ticks
normally, which causes a burst of ticks until it is "caught up" in time
where it should be. This behavior is not always desired, however, so
this commit adds the `MissedTickBehavior` enum to the `time` module to
allow users to specify what behavior they'd like `Interval` to exhibit
in such situations.

Closes tokio-rs#3574
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Apr 23, 2021
@eb-64-64
Copy link
Contributor Author

eb-64-64 commented Apr 23, 2021

The CI is failing because of a broken intradoc link. I added a link to Stream in the docs, which was previously unlinked, but I linked it to std::stream::Stream, which is nightly for now. Since std::stream::Stream isn't stabilized for now, I see a two options:

  • Leave it unlinked until it is stabilized
  • Link to the futures one for now, then change it to the std one once it's stabilized

Which should I do? Either way, should I create an issue to track this?

@Darksonn
Copy link
Contributor

Just leave it unlinked.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the entire PR, because it spends a lot of time talking about the executor being blocked. This would typically not be the reason that ticks get behind (but sure, it could be). I'd like to discuss the details here before I continue reviewing.

tokio/src/time/interval.rs Outdated Show resolved Hide resolved
tokio/src/time/interval.rs Outdated Show resolved Hide resolved
tokio/src/time/interval.rs Outdated Show resolved Hide resolved
tokio/src/time/interval.rs Outdated Show resolved Hide resolved
tokio/src/time/interval.rs Outdated Show resolved Hide resolved
tokio/src/time/interval.rs Outdated Show resolved Hide resolved
Comment on lines 154 to 165
/// ```text
/// Expected ticks: | 1 | 2 | 3 | 4 | 5 | 6 |
/// Actual ticks: | work -----| delay | work | work | work -| work -----|
// Poll behavior: | | | | | | | |
// | | | | | | | |
// Ready(s) | | Ready(s + 2p) | | | |
// Pending | Ready(s + 3p) | | |
// Ready(s + p) Ready(s + 4p) | |
// Ready(s + 5p) |
// Ready(s + 6p)
// * Where `s` is the start time and `p` is the period
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand this diagram.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the new commit help to simplify? The poll behavior stuff was to help me figure out how to implement it. I left it in there for documentation purposes, but it seems to add more confusion. I used the same type of diagram that @Lindenk used in #3574. The idea is that 'work' is how long we spend doing, well, actual work, the dashes are time we spend (asynchronously) waiting for the tick, and 'delay' is representative of a situation where an Interval would be delayed from ticking.

So, we would expect the ticks to proceed along like diagrammed in the first line ('Expected ticks:'). But if the Interval is delayed, the bottom line ('Actual ticks:') shows how the Interval would behave. In this specific case, notice that there is no delay (no dashes) in the firing of ticks after the long 'delay' tick. That's because the specific behavior this diagram represents doesn't wait for ticks until it is "caught up" in time to where it should be. In this case, after 1 normal tick, one excessively long tick, and 2 ticks done without any delay, the 5th tick starts late, but finishes its work before the next tick was supposed to fire, so it waits for a little bit before tick 6 fires.

tokio/src/time/interval.rs Outdated Show resolved Hide resolved
/// a multiple of `period` from `delay`.
///
/// [`tick`]: Interval::tick
Delay,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand what this one does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you're new to this code, can you help me clarify the documentation so that it's not confusing? The gist of it is this:

use tokio::time::{interval, Duration};

let mut interval = interval(Duration::from_millis(50));

task_that_takes_more_than_50_millis().await;
// The `Interval` has missed a tick

// Since we have exceeded our timeout, this will resolve immediately
interval.tick().await;

// But this one, rather than also resolving immediately, as might happen
// with the `Burst` behavior (depending on how long the `Interval` was
// delayed), will not resolve until 50ms after the call to `tick` up above.
// That is, in `tick`, when we recognize that we missed a tick, we schedule
// the next tick to happen 50ms (or whatever the `period` is) from right
// then, not from when were were *supposed* to tick
interval.tick().await;

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand now.

eb-64-64 and others added 5 commits May 14, 2021 14:48
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
@eb-64-64
Copy link
Contributor Author

It seems like the biggest thing that needs to be done to move the PR forward is to overhaul the documentation. I'm going to be really time-constrained the next couple of days, so it might take about a week. If you have anything you want me to include, or any other feedback or pointers, I'd love to hear it! Otherwise, I'll just be working on it as much as I can.

@Darksonn
Copy link
Contributor

The diagrams are all much better now. I think the bottom part of them was confusing me. Don't worry about the time it takes — we are not in a hurry.

@eb-64-64
Copy link
Contributor Author

eb-64-64 commented May 27, 2021

Okay, I added simple code examples, like the one I wrote for you, and reviewed the documentation in hopes of making it as simple and clear as possible. Since you're newer to the code, I'd love to hear how understandable everything is to you now. Since this is kind of hard to understand, I want to make the documentation as crystal-clear as possible. Thanks!

tokio/src/time/interval.rs Outdated Show resolved Hide resolved
@Darksonn Darksonn merged commit e979ad7 into tokio-rs:master Jun 16, 2021
@eb-64-64 eb-64-64 deleted the missed-tick-behavior branch June 18, 2021 00:16
@Darksonn Darksonn mentioned this pull request Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-time Module: tokio/time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide behavior options for time::Interval
2 participants