Skip to content

Commit 6db064c

Browse files
committed
gbn: serialize interval-aware ticker resets
Protect IntervalAwareForceTicker lifecycle operations with a mutex so Reset, ResetWithInterval, Stop, and ForceTick cannot replace or stop the underlying time.Ticker concurrently. The keepalive timeout compatibility tests exercise concurrent send and receive paths that both reset the ping/pong tickers. In normal operation, sendPacketsForever can reset the pong ticker when starting a keepalive ping while receivePacketsForever resets the ping ticker when a packet arrives. The interval-aware ticker implementation mutated its ticker, quit channel, and interval fields without synchronization, so the race detector could observe concurrent reads and writes in the reset path. This change adds a dedicated lock around ticker lifecycle updates and uses a locked helper for interval resets. That preserves the existing behavior while making the ticker safe under the concurrent reset pattern used by the keepalive logic. Why the fix was needed: The new keepalive compatibility tests made two goroutines hit the ticker harder: - sendPacketsForever() resets the pong ticker when it sends a keepalive ping. - receivePacketsForever() resets the ping ticker when packets arrive. IntervalAwareForceTicker internally stops and recreates its time.Ticker, replaces quit, and updates interval. Before the fix, those fields were changed without a mutex, so overlapping resets could race. go test -race caught exactly that in ticker.go:143-162. So the fix was needed because: - the ticker was not safe under concurrent reset/stop operations - the keepalive code legitimately does concurrent resets - race CI was correctly flagging unsynchronized internal state mutation The mutex makes those lifecycle operations atomic and keeps the implementation behavior the same.
1 parent 47db8cb commit 6db064c

1 file changed

Lines changed: 31 additions & 3 deletions

File tree

gbn/ticker.go

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ import (
1515
type IntervalAwareForceTicker struct {
1616
isActive uint32 // used atomically
1717

18+
// opsMu serializes lifecycle operations that replace or stop the
19+
// underlying clock ticker.
20+
opsMu sync.Mutex
21+
1822
// Force is used to force-feed a ticks into the ticker. Useful for
1923
// debugging when trying to wake an event.
2024
Force chan time.Time
@@ -129,6 +133,9 @@ func (t *IntervalAwareForceTicker) Pause() {
129133
//
130134
// NOTE: Part of the Ticker interface.
131135
func (t *IntervalAwareForceTicker) Stop() {
136+
t.opsMu.Lock()
137+
defer t.opsMu.Unlock()
138+
132139
t.Pause()
133140
t.ticker.Stop()
134141
close(t.quit)
@@ -138,6 +145,17 @@ func (t *IntervalAwareForceTicker) Stop() {
138145
// ResetWithInterval restarts the ticker with the given interval, causing the
139146
// next clock tick to occur in the given interval.
140147
func (t *IntervalAwareForceTicker) ResetWithInterval(newInterval time.Duration) {
148+
t.opsMu.Lock()
149+
defer t.opsMu.Unlock()
150+
151+
t.resetWithIntervalLocked(newInterval)
152+
}
153+
154+
// resetWithIntervalLocked restarts the ticker with the given interval.
155+
// The caller must hold opsMu.
156+
func (t *IntervalAwareForceTicker) resetWithIntervalLocked(
157+
newInterval time.Duration) {
158+
141159
// Shutdown the internal clock ticker without changing isActive.
142160
t.ticker.Stop()
143161
close(t.quit)
@@ -159,14 +177,20 @@ func (t *IntervalAwareForceTicker) ResetWithInterval(newInterval time.Duration)
159177
// Reset restarts the ticker interval, causing the next clock tick to occur in
160178
// the configured interval.
161179
func (t *IntervalAwareForceTicker) Reset() {
162-
t.ResetWithInterval(t.interval)
180+
t.opsMu.Lock()
181+
defer t.opsMu.Unlock()
182+
183+
t.resetWithIntervalLocked(t.interval)
163184
}
164185

165186
// ForceTick force feeds an event into the ticker channel and resets the
166187
// internal clock ticker causing the next clock tick to occur in the configured
167188
// interval.
168189
func (t *IntervalAwareForceTicker) ForceTick() {
169-
t.Reset()
190+
t.opsMu.Lock()
191+
t.resetWithIntervalLocked(t.interval)
192+
t.opsMu.Unlock()
193+
170194
t.Force <- time.Now()
171195
}
172196

@@ -183,7 +207,11 @@ func (t *IntervalAwareForceTicker) LastTimedTick() time.Time {
183207
// NextTickIn returns the approximate duration until the next timed tick will
184208
// occur.
185209
func (t *IntervalAwareForceTicker) NextTickIn() time.Duration {
186-
nextTick := t.LastTimedTick().Add(t.interval)
210+
t.opsMu.Lock()
211+
interval := t.interval
212+
t.opsMu.Unlock()
213+
214+
nextTick := t.LastTimedTick().Add(interval)
187215
durationToNextTick := time.Until(nextTick)
188216
if durationToNextTick < 0 {
189217
return 0

0 commit comments

Comments
 (0)