Reuse time.Timer in ack and rtx timer utilities #336
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #336 +/- ##
==========================================
+ Coverage 81.13% 81.18% +0.04%
==========================================
Files 49 49
Lines 3250 3247 -3
==========================================
- Hits 2637 2636 -1
+ Misses 470 469 -1
+ Partials 143 142 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@paulwe is this for performance by any chance? |
|
@edaniels yes, we're seeing high |
ack_timer.go
Outdated
| case ackTimerStarted: | ||
| t.state = ackTimerStopped | ||
| defer t.observer.onAckTimeout() | ||
| case ackTimerClosed: |
There was a problem hiding this comment.
is it worth emptying t.cancel here as well?
There was a problem hiding this comment.
state and writes to the cancel channel are synced.
- if we are in this critical section and the state is
startedwe cannot have written tocancel. - if we change the state from
startedtostoppedin this critical section then callstopafterward the state will bestoppedand we won't won't write tocancel. cancelcould be dirty if the state isclosedbut we won't reuse the timer later so we don't care.
ack_timer.go
Outdated
| } | ||
|
|
||
| cancelCh := make(chan struct{}) | ||
| t.state = ackTimerStarted |
There was a problem hiding this comment.
Is there a chance of race here? Is there a need to drain timer channel or t.cancel?
Thinking aloud
stop()called: that will write tot.canceland set state tostopped- Assume, the go routine below has not drained
t.cancelyet. start()happens: this will try to restart the timer as state isstopped- This will launch another go routine. Will this clash with go routine in Step 2?
There was a problem hiding this comment.
good catch. invocations of the callback are interchangeable and for this to happen the time between calls has to be practically zero so we can eat the extra cancel and the earlier goroutine will serve as the timeout for the latest call to start.
paulwe
force-pushed
the
timer-reuse
branch
2 times, most recently
from
415e0bb
to
9fb4b50
Compare
No description provided.