-
-
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 race condition in timer drop #3229
Conversation
Dropping a timer on the millisecond that it was scheduled for, when it was on the pending list, could result in a panic previously, as we did not record the pending-list state in cached_when. Hopefully fixes: ZcashFoundation/zebra#1452
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.
couple of very minor nits, but this looks correct to me!
tokio/tests/time_sleep.rs
Outdated
use std::sync::{Arc, Mutex}; | ||
use std::task::Context; | ||
|
||
let paniced = Arc::new(AtomicBool::new(false)); |
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.
nit: should be panicked
tokio/src/time/driver/wheel/mod.rs
Outdated
unsafe { | ||
item.set_cached_when(u64::max_value()); | ||
} |
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.
There's a comment a few lines above (on the match
) that seems to imply that mark_pending
updates cached_when
; this isn't actually the case. Maybe we should update that comment to indicate that this is where cached_when
is updated?
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.
On second thought, mark_pending
is probably the right place to do this update (it does this update when a timer is found to have been rescheduled)
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.
I was wondering about that too...this is the only place we call mark_pending
, so it makes sense. Then, we can remove the set_cached_when
methods.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@carllerche did you want to take a look at this before it merges? it looks good to me but i thought you might want to sign off... |
@hawkw I trust you 👍 |
Updates the tokio dependency to the commit that merged tokio-rs/tokio#3229, which should fix the time wheel panic in ZcashFoundation#1452.
Updates the tokio dependency to the commit that merged tokio-rs/tokio#3229, which should fix the time wheel panic in #1452.
I've been running |
@teor2345 great news. Thanks for checking 👍 |
Dropping a timer on the millisecond that it was scheduled for, when it was on
the pending list, could result in a panic previously, as we did not record the
pending-list state in cached_when.
Hopefully fixes: ZcashFoundation/zebra#1452
Motivation
Solution