-
Notifications
You must be signed in to change notification settings - Fork 675
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
Add StoppedTimer helper #3280
Conversation
As per @StephenButtolph's comment: ava-labs/subnet-evm#1166 (comment) Co-authored-by: Stephen Buttolph <[email protected]>
b384723
to
7ed9df8
Compare
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.
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" |
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.
(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.
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.
Done
utils/timer/stopped_timer.go
Outdated
// 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). | ||
|
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) You've accidentally broken the comment here. I'm not sure if doc parsers will fix that or not.
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.
Done
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.
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 { |
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 think this should also be used here:
avalanchego/network/throttling/inbound_resource_throttler.go
Lines 109 to 114 in 6626d2b
// Satisfy invariant that timer is stopped and drained. | |
timer := time.NewTimer(0) | |
if !timer.Stop() { | |
<-timer.C | |
} | |
return timer |
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.
Done
Co-authored-by: Stephen Buttolph <[email protected]>
As per @StephenButtolph's recent comment.