-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 wake-up with interval on Ready
#5553
time: fix wake-up with interval on Ready
#5553
Conversation
c48f061
to
b2d67ef
Compare
Ready
(#5551)Ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the PR!
The implementation looks ok to me, but I've asked @carllerche to take a look at whether there's anything special to think about when skipping reregister
.
I have some comments regarding your test.
tokio/tests/time_interval.rs
Outdated
@@ -209,3 +209,76 @@ fn poll_next(interval: &mut task::Spawn<time::Interval>) -> Poll<Instant> { | |||
fn ms(n: u64) -> Duration { | |||
Duration::from_millis(n) | |||
} | |||
|
|||
mod tmp_tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I intended to keep the specific helper struct with the Stream
implementation and the test together, but I guess it's not needed. I removed it.
tokio/tests/time_interval.rs
Outdated
use std::{ | ||
pin::Pin, | ||
task::{Context, Poll}, | ||
time::Instant, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this import style in Tokio.
use std::{ | |
pin::Pin, | |
task::{Context, Poll}, | |
time::Instant, | |
}; | |
use std::pin::Pin; | |
use std::task::{Context, Poll}; | |
use std::time::Instant; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't currently use it.
As an aside, I wish we did and would take a PR that did a big pass-through of Tokio to update all imports... though others should chime in before anyone does the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK thanks for letting me know - updated.
tokio/tests/time_interval.rs
Outdated
timer: crate::time::interval(std::time::Duration::from_millis(10)), | ||
}; | ||
|
||
pin_mut!(stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pin_mut!(stream); | |
tokio::pin!(stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tokio/tests/time_interval.rs
Outdated
} | ||
} | ||
|
||
#[tokio::test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should make the test execute much faster by having it instantly skip forward to the next timer whenever a sleep happens.
#[tokio::test] | |
#[tokio::test(start_paused = true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK nice! With this, do I need to manually use any tokio::time::advance
, or will it automatically skip forward?
tokio/tests/time_interval.rs
Outdated
// Schedule this task for wake-up | ||
cx.waker().wake_by_ref(); | ||
Poll::Pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, you're not actually testing whether it emits a wakeup or not. This test would pass either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the test was not complete but more a manual thing until I hear back on the approach. Sorry that I did not explain that better. I added two tests now which tease out the difference.
tokio/src/runtime/time/entry.rs
Outdated
@@ -543,6 +543,17 @@ impl TimerEntry { | |||
} | |||
} | |||
|
|||
pub(crate) fn reset_without_reregister(mut self: Pin<&mut Self>, new_time: Instant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is very similar to reset
. Maybe reset
should take an argument reregister: bool
. I'm not 100% sure, but it is worth considering given the unsafe
in the fn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, good point. I checked and there were not too many usages of TimerEntry::reset
to update, so I added the boolean flag and removed reset_without_reregister
.
tokio/src/time/sleep.rs
Outdated
/// To call this method, you will usually combine the call with | ||
/// [`Pin::as_mut`], which lets you call the method without consuming the | ||
/// `Sleep` itself. | ||
pub fn reset_without_reregister(self: Pin<&mut Self>, deadline: Instant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new public function should be a high bar. Either the existing behavior is incorrect, and we should fix it or accept it the way it is and evaluate adding a new API.
My concern with reset_without_reregister
is that it makes the caller decide. Which variant should they use? Choosing one requires knowing about "waking," which isn't required for using Tokio as a beginner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm that is a good point 🤔 I do think that users of tokio::time::Sleep
would always want the re-registering variant. The non-re-registering is only needed for the specific use case in tokio::time::Interval
. I changed the visibility to pub(crate)
. Would that be OK?
tokio/tests/time_interval.rs
Outdated
use std::{ | ||
pin::Pin, | ||
task::{Context, Poll}, | ||
time::Instant, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't currently use it.
As an aside, I wish we did and would take a PR that did a big pass-through of Tokio to update all imports... though others should chime in before anyone does the work.
82b9a7f
to
c2586d6
Compare
When `tokio::time::Interval::poll_tick()` returns `Poll::Pending`, it schedules itself for being woken up again through the waker of the passed context, which is correct behavior. However when `Poll::Ready(_)` is returned, the interval timer should be reset but not scheduled to be woken up again as this is up to the caller. This commit fixes the bug by introducing a `reset_without_reregister` method on `TimerEntry` which is called by `Intervall::poll_tick(cx)` in case the delay poll returns `Poll::Ready(_)`.
c2586d6
to
8dffc8d
Compare
@Darksonn or @carllerche would you have time to check the updated version? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.26.0` -> `1.27.0` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.26.0` -> `1.27.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.27.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.27.0): Tokio v1.27.0 [Compare Source](tokio-rs/tokio@tokio-1.26.0...tokio-1.27.0) ##### 1.27.0 (March 27th, 2023) This release bumps the MSRV of Tokio to 1.56. ([#​5559]) ##### Added - io: add `async_io` helper method to sockets ([#​5512]) - io: add implementations of `AsFd`/`AsHandle`/`AsSocket` ([#​5514], [#​5540]) - net: add `UdpSocket::peek_sender()` ([#​5520]) - sync: add `RwLockWriteGuard::{downgrade_map, try_downgrade_map}` ([#​5527]) - task: add `JoinHandle::abort_handle` ([#​5543]) ##### Changed - io: use `memchr` from `libc` ([#​5558]) - macros: accept path as crate rename in `#[tokio::main]` ([#​5557]) - macros: update to syn 2.0.0 ([#​5572]) - time: don't register for a wakeup when `Interval` returns `Ready` ([#​5553]) ##### Fixed - fs: fuse std iterator in `ReadDir` ([#​5555]) - tracing: fix `spawn_blocking` location fields ([#​5573]) - time: clean up redundant check in `Wheel::poll()` ([#​5574]) ##### Documented - macros: define cancellation safety ([#​5525]) - io: add details to docs of `tokio::io::copy[_buf]` ([#​5575]) - io: refer to `ReaderStream` and `StreamReader` in module docs ([#​5576]) [#​5512]: tokio-rs/tokio#5512 [#​5514]: tokio-rs/tokio#5514 [#​5520]: tokio-rs/tokio#5520 [#​5525]: tokio-rs/tokio#5525 [#​5527]: tokio-rs/tokio#5527 [#​5540]: tokio-rs/tokio#5540 [#​5543]: tokio-rs/tokio#5543 [#​5553]: tokio-rs/tokio#5553 [#​5555]: tokio-rs/tokio#5555 [#​5557]: tokio-rs/tokio#5557 [#​5558]: tokio-rs/tokio#5558 [#​5559]: tokio-rs/tokio#5559 [#​5572]: tokio-rs/tokio#5572 [#​5573]: tokio-rs/tokio#5573 [#​5574]: tokio-rs/tokio#5574 [#​5575]: tokio-rs/tokio#5575 [#​5576]: tokio-rs/tokio#5576 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yNC41IiwidXBkYXRlZEluVmVyIjoiMzUuMjQuNSJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1838 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
With tokio 1.27.0, the Interval type changed in backwards incompatible ways, now necessitating a call to Interval::reset to schedule another wake up [0]. This change makes sure that we follow the new procedure. [0]: tokio-rs/tokio#5553
With tokio 1.27.0, the Interval type changed in backwards incompatible ways, now necessitating a call to Interval::reset to schedule another wake up [0]. This change makes sure that we follow the new procedure. [0]: tokio-rs/tokio#5553
For what it's worth, I would say that this change is most certainly backwards compatibility breaking, as now an additional call is necessary to get the previous behavior when using the |
The more appropriate course of action, from my perspective, would have been to introduce a new method with the new behavior, deprecate the existing |
@d-e-s-o As far as I understand, your code was already broken. The interval would not update the waker when it returns To add to that, your fix is incorrect. The To be clear, this is how all poll methods work. It's not something special to
This change is a bug fix. Bug fixes are always breaking changes. Of course, there is always a trade-off between fixing bugs and keeping broken behavior. |
Ah okay. Thanks for the explanation! |
Motivation
When
tokio::time::Interval::poll_tick()
returnsPoll::Pending
, it schedules itself for being woken up again through the waker of the passed context, which is correct behavior. However whenPoll::Ready(_)
is returned, the interval timer should be reset but not scheduled to be woken up again as this is up to the caller.Currently, resetting the timer always involves reregistering the last waker which polled the internal
Sleep
to be woken up on completion.Solution
This PR fixes the bug by introducing a
reset_without_reregister
method onTimerEntry
which is called byIntervall::poll_tick(cx)
in case the delay poll returnsPoll::Ready(_)
.Closes: #5551