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: fix time::advance() with sub-ms durations #3852

Merged
merged 7 commits into from
Jun 14, 2021
Merged

Conversation

carllerche
Copy link
Member

Update the advance logic to factor in the timer's ms rounding.

Fixes #3837

tokio/tests/time_pause.rs Show resolved Hide resolved
tokio/tests/time_pause.rs Outdated Show resolved Hide resolved
tokio/tests/time_pause.rs Outdated Show resolved Hide resolved
tokio/src/time/clock.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Does this PR affect #3763?

@carllerche
Copy link
Member Author

Does this PR affect #3763?

It should not.

@carllerche carllerche changed the base branch from master to tokio-1.6.x June 11, 2021 21:06
Update the advance logic to factor in the timer's ms rounding.

Fixes #3837
@carllerche
Copy link
Member Author

I rebased on 1.6.x so that this can go out as a patch, once it is ready.

carllerche and others added 2 commits June 11, 2021 14:09
Co-authored-by: sb64 <53383020+sb64@users.noreply.github.com>
@carllerche
Copy link
Member Author

Pushed a change. Instead of using a sleep in time::advance, this fixes the root of the issue. When futures passed to Runtime::block_on are woken, it bypassed all the machinery around advancing time. By intercepting wakes in the time driver, we know when the block_on task is woken and skip advancing time in that case.

tokio/src/time/driver/mod.rs Outdated Show resolved Hide resolved
tokio/src/time/driver/mod.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member Author

Applied feedback.

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.

Looks good to me!

@carllerche carllerche merged commit 18779aa into tokio-1.6.x Jun 14, 2021
@carllerche carllerche deleted the fix-3837 branch June 14, 2021 23:36
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Jun 16, 2021
These don't actually *need* to advance the mock clock --- they are just
intended to make the current task yield so the background task can
observe updates. Replacing them with `yield_now` expresses intent more
accurately.

Also, this seems to fix the test failing after upgrading to tokio
1.6.2+, presumably related to tokio-rs/tokio#3852. I'm still trying to
figure out _why_ exactly this fixes the test (and why it breaks in the
first place), but at least this PR will get CI passing on Tokio 1.7.0.

See here for details:
#1079 (comment)
olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this pull request Jun 16, 2021
These don't actually *need* to advance the mock clock --- they are just
intended to make the current task yield so the background task can
observe updates. Replacing them with `yield_now` expresses intent more
accurately.

Also, this seems to fix the test failing after upgrading to tokio
1.6.2+, presumably related to tokio-rs/tokio#3852. I'm still trying to
figure out _why_ exactly this fixes the test (and why it breaks in the
first place), but at least this PR will get CI passing on Tokio 1.7.0.

See here for details:
#1079 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

time::advance advances too far when given a Duration of sub-millisecond granularity
3 participants