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

Add StoppedTimer helper #3280

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Add StoppedTimer helper #3280

merged 2 commits into from
Aug 8, 2024

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 8, 2024

@marun marun self-assigned this Aug 8, 2024
As per @StephenButtolph's comment:
ava-labs/subnet-evm#1166 (comment)

Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
Copy link
Contributor

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

Approving with minor requests that don't require re-review.

@@ -11,6 +11,8 @@ import (
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/utils/set"
"github.com/ava-labs/avalanchego/utils/timer/mockable"

utils_timer "github.com/ava-labs/avalanchego/utils/timer"
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional, subjective) When a package and a local variable clash, one recommended approach is to use the pkg suffix so this would be timerpkg.

The underscore in the name is what jumped out at me as out of the ordinary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// This means that after calling Reset there will be no events on the
// channel until the timer fires (at which point there will be exactly
// one event sent to the channel).

Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) You've accidentally broken the comment here. I'm not sure if doc parsers will fix that or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

lgtm after the few nits are addressed

// It enables re-using the timer across loop iterations without
// needing to have the first loop iteration perform any == nil checks
// to initialize the first invocation.
func StoppedTimer() *time.Timer {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should also be used here:

// Satisfy invariant that timer is stopped and drained.
timer := time.NewTimer(0)
if !timer.Stop() {
<-timer.C
}
return timer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@StephenButtolph StephenButtolph added this to the v1.11.11 milestone Aug 8, 2024
@StephenButtolph StephenButtolph added the cleanup Code quality improvement label Aug 8, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Aug 8, 2024
Merged via the queue into master with commit ab504e9 Aug 8, 2024
21 checks passed
@StephenButtolph StephenButtolph deleted the add-stopped-timer branch August 8, 2024 17:34
marun added a commit that referenced this pull request Aug 8, 2024
Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code quality improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants