-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
time: avoid panic on time::advance(Duration::MAX)
#7381
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
Conversation
| inner.base += duration; | ||
| inner.base = inner | ||
| .base | ||
| .checked_add(duration) | ||
| .unwrap_or(far_future); |
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 avoids the panic, but I wonder how correct this is. Does the timer still behave correctly after this call?
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.
tokio/tokio/src/runtime/time/mod.rs
Lines 270 to 286 in 2440d11
| while let Some(entry) = lock.wheel.poll(now) { | |
| debug_assert!(unsafe { entry.is_pending() }); | |
| // SAFETY: We hold the driver lock, and just removed the entry from any linked lists. | |
| if let Some(waker) = unsafe { entry.fire(Ok(())) } { | |
| waker_list.push(waker); | |
| if !waker_list.can_push() { | |
| // Wake a batch of wakers. To avoid deadlock, we must do this with the lock temporarily dropped. | |
| drop(lock); | |
| waker_list.wake_all(); | |
| lock = self.inner.lock(); | |
| } | |
| } | |
| } |
tokio/tokio/src/runtime/time/wheel/mod.rs
Lines 143 to 167 in 2440d11
| pub(crate) fn poll(&mut self, now: u64) -> Option<TimerHandle> { | |
| loop { | |
| if let Some(handle) = self.pending.pop_back() { | |
| return Some(handle); | |
| } | |
| match self.next_expiration() { | |
| Some(ref expiration) if expiration.deadline <= now => { | |
| self.process_expiration(expiration); | |
| self.set_elapsed(expiration.deadline); | |
| } | |
| _ => { | |
| // in this case the poll did not indicate an expiration | |
| // _and_ we were not able to find a next expiration in | |
| // the current list of timers. advance to the poll's | |
| // current time and do nothing else. | |
| self.set_elapsed(now); | |
| break; | |
| } | |
| } | |
| } | |
| self.pending.pop_back() | |
| } |
Good question, I read the code and I realized that the time driver keeps polling the wheel until wheel.elapsed == now.
So I think that even though the wheel rotates a lot of full revolution because of far_future, it still works correctly.
I also added more tests to ensure the correct behavior after a huge advance.
| // Retrieve `far_future` before acquiring the mutex to prevent deadlock, | ||
| // as `Instant::far_future()` also acquires a mutex internally. | ||
| let far_future = Instant::far_future().into_std(); |
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 would ideally like to avoid acquiring the mutex twice every time just for supporting this one case.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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.
inner.base = inner
.base
.checked_add(duration)
.unwrap_or_else(|| inner.base + <30 years>);To bypass the Mutex manually add 30 years on overflow, but this still panics after many times of time::advance(Duration::MAX), I don't have perfect idea yet.
Let me ask the issue author about the real (original) requirement, maybe there will be a better solution.
|
Superseded by #7394 |
Motivation
close #7380
Solution
The solution is straightforward.
Note that
Instant::far_future()also acquires the mutex internally, so we have to fetch it before theself.inner.lock().