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

Improve timer functionality #87

Merged
merged 3 commits into from Aug 21, 2022
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Aug 20, 2022

This PR fixes two outstanding issues with timers:

  • Fix panic in Timer with large durations #86, where large Durations can cause the Timer code to panic as a result of unchecked addition. This PR checks to ensure that the addition does not overflow; if it does, it returns an Instant in the far future.
  • A blank Timer that never fires #42, suggesting that a blank Timer could exist that never fires. This not only adds a blank method that creates a timer that isn't registered in the Reactor, but also uses this method to implement Default on Timer.

Closes #86
Closes #42

src/lib.rs Outdated
Comment on lines 387 to 391
impl Default for Timer {
fn default() -> Self {
Timer::blank()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinions, but I personally feel it is a bit odd that a timer that is never resolved is the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three options I see are:

  • Leave the Timer without a Default impl.
  • Have the default impl never fire.
  • Have the default impl fire immediately.

Now that I think about it, the third options seems to be the least footgun-like.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm starting to think that the first option is better. There aren't many instances where the Default impl of a timer is called anyhow (unless you're storing them in a TinyVec), and the ones where they would be feel unavoidably like a footgun. I'll just not have a Default impl; we can discuss it later if we need to.

Copy link
Collaborator

@taiki-e taiki-e Aug 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it seems better to postpone the Default implementation until someone actually asks for it.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix panic in Timer with large durations A blank Timer that never fires
2 participants