Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
tick()guards only the first delivery time:The two re-arm sites inside
tick.rsuse unchecked+, so a duration that passes thatguard can still overflow when the next tick is computed:
crossbeam-channel/src/flavors/tick.rs:92(try_recv):Align(now + self.duration)crossbeam-channel/src/flavors/tick.rs:120(recv):Align(delivery_time.0.max(now) + self.duration)Any duration larger than half the representable
Instantrange but under the whole rangelands in that window.
crossbeam_channel::tick(Duration::from_secs(1 << 62))followed byrecv()panics instantly, because the CAS that computes the next delivery time runs beforethe sleep. That is the opposite of the "wait forever on very large timeout" behavior that
commit 6951bc9 intended: that commit moved the saturating deadline conversion out of
tick::Channel::newinto thechecked_addinchannel.rs, and the two re-arm sites wereleft unchecked.
Fix
A small saturating helper in
tick.rs, used at both sites. An unrepresentable next tickmeans "effectively never", matching the
None => never()arm intick(). Halving ratherthan
unwrap_or(base)avoids re-arming in the past, which would make the channel fire in atight loop.
Evidence
Before, on current master (38dacb4):
After:
cargo fmt --checkandcargo clippy -p crossbeam-channel --lib --testsare clean.Not fixed here
at()/after()in theatflavor are single-shot and do no re-arm arithmetic, so they areuntouched. I also left the
never()fallback inchannel.rsalone rather than widening it,to keep the change minimal.