Skip to content

Conversation

lexnv
Copy link

@lexnv lexnv commented Mar 4, 2025

This PR fixes an overflow produced by the Instant::poll_tick function when called for an instant set with a period from u64::MAX.

For example, timers initiated like the following would panic because there's no room to add the 5 ms duration:

let mut timer = tokio::time::interval(std::time::Duration::from_secs(u64::MAX));

cc paritytech/polkadot-sdk#7793

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Mar 4, 2025
@Darksonn
Copy link
Contributor

Darksonn commented Mar 4, 2025

Thanks for the PR. Please add a test that would have caught this.

lexnv added 3 commits March 4, 2025 13:39
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Author

lexnv commented Mar 4, 2025

Thanks for the swift review! 🙏

I've added a test and patched the missed tick behavior as well

}

#[tokio::test(start_paused = true)]
#[tokio::test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this test use start_paused?

Copy link
Author

Choose a reason for hiding this comment

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

We need a bit of delay between two subsequent poll_tick calls (5ms) to enter the misstick behavior and trigger the overflow panic

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, a test that takes 2 seconds to complete is not acceptable. I'm quite certain that the test works just fine with start_paused - the parameter should not change the behavior of tests that rely on time other than making them run faster.

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.

2 participants