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: do not panic on Interval::poll_tick if the sum of the timeout and period overflow #6487

Merged
merged 6 commits into from
Apr 25, 2024

Conversation

jxs
Copy link
Member

@jxs jxs commented Apr 15, 2024

Motivation

when calling tokio::time::interval(Duration::MAX) runtime panics with:

overflow when adding duration to instant

see reproduction here. Panic happens when adding timeout to to the delay deadline.

Solution

This PR addresses the panic by calling Instant::checked_add falling back to Instant::far_future() when it overflows. If needed I can add a test for it and update the documentation mentioning this behaviour.

Thanks!

if the sum of the timeout and period overflow
@mox692 mox692 added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Apr 16, 2024
@Darksonn
Copy link
Contributor

Thanks. Can you add a test to tests/time_interval.rs? Otherwise this looks good to me.

@b3t33
Copy link

b3t33 commented Apr 18, 2024

Using Instant::far_future() seems like a reasonable fallback while saturating_add is not available. Alternatively, a good approximation for saturating_add could be the following:

fn saturating_add(lhs: Instant, rhs: Duration) -> Instant {
    if let Some(out) = lhs.checked_add(rhs) {
        out
    } else {
        saturate(lhs)
    }
}

#[cold]
fn saturate(mut instant: Instant) -> Instant {
    let mut secs = 1_u64 << 63;
    while secs > 0 {
        if let Some(incr) = instant.checked_add(Duration::from_secs(secs)) {
            instant = incr;
        }
        secs >>= 1;
    }
    let mut nanos = 1_u64 << 30;
    while nanos > 0 {
        if let Some(incr) = instant.checked_add(Duration::from_nanos(nanos)) {
            instant = incr;
        }
        nanos >>= 1;
    }
    instant
}

see #https://github.com/b3t33/tokio/commit/2e9743b74d097c14ec3223f909d12daa12f8532a for a possible integration into src/time/instant.rs.

@Darksonn
Copy link
Contributor

Instant::far_future() is sufficient.

@wathenjiang
Copy link
Contributor

Is it a good idea to add some explanation about this overflow behavior on the tokio:: time:: interval.

@Darksonn
Copy link
Contributor

I don't think it's necessary, but it also doesn't hurt to mention that.

@jxs
Copy link
Member Author

jxs commented Apr 23, 2024

thanks Alice, added a test

@jxs jxs requested a review from Darksonn April 23, 2024 18:32
timeout + self.period
timeout
.checked_add(self.period)
.unwrap_or(Instant::far_future())
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by review: this may make sense as unwrap_or_else 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Paolo, updated!

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've verified that the test does fail without this change. Thanks!

@Darksonn
Copy link
Contributor

It seems there is a clippy failure.

error: redundant closure
   --> tokio/src/time/interval.rs:485:33
    |
485 |                 .unwrap_or_else(|| Instant::far_future())
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `Instant::far_future`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
    = note: `-D clippy::redundant-closure` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::redundant_closure)]`

@jxs
Copy link
Member Author

jxs commented Apr 24, 2024

Stress test is failing, but seems unrelated

@Darksonn
Copy link
Contributor

Yes, see the CI issue above.

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.

6 participants