runtime: fix ticker not stopping when Stop races with its callback (#5487)

* runtime: fix ticker not stopping when Stop races with its callback

On the threads and cores schedulers, timer callbacks run concurrently
with user goroutines.

If Stop (or Reset) was called in the window after the node was popped
but before its callback re-added it, removeTimer would not find the
timer in the queue, so it would be re-added to timer anyway.

Track timers whose callback is currently running in a firing list, and
have removeTimer mark a firing timer as stopped so its callback does not
re-add it.

Also make the timers test drain robust: allow a possible in-flight tick
delivered concurrently with Stop to settle before draining the channel.

Signed-off-by: deadprogram <ron@hybridgroup.com>

* runtime: fix Stop/Reset semantics when racing a firing timer callback

Address review feedback on the ticker Stop-race fix. On the threads and
cores schedulers a timer callback runs concurrently with user goroutines,
which left several problems:

- removeTimer reported a firing timer as successfully removed, so
  Stop/Reset could return true even though the callback had already
  started (wrong semantics, notably for AfterFunc). firingTimerStop now
  returns a bool and removeTimer no longer hands the still-firing node
  back to resetTimer.

- The periodic advance (when += period) ran in timerCallback outside the
  scheduler timer lock. Move it into each scheduler's reAddTimer, under
  the lock and after the stopped check, so a concurrent Reset can't have
  its freshly-queued deadline corrupted.

- resetTimer now sets when/period after removeTimer for the same reason.

Add testdata/timer_stop_reset_race.go and TestTimerStopResetRace, which
reproduce the stop-while-firing and reset-while-firing races via the
runtime timer linkname hooks.

Signed-off-by: deadprogram <ron@hybridgroup.com>

* runtime: fix timer Stop/Reset race with firing periodic timers

Signed-off-by: deadprogram <ron@hybridgroup.com>

---------

Signed-off-by: deadprogram <ron@hybridgroup.com>

main_test.go

@@ -245,6 +245,21 @@ func TestBuild(t *testing.T) {
 	}
 }
 
+// TestTimerStopResetRace checks that stopping or resetting a timer while its
+// callback is running behaves correctly. It reaches into the runtime timer
+// hooks via //go:linkname and relies on the threads scheduler, which is only
+// used on these hosts.
+func TestTimerStopResetRace(t *testing.T) {
+	t.Parallel()
+
+	switch runtime.GOOS {
+	case "darwin", "linux":
+	default:
+		t.Skipf("host GOOS %s does not use the threads scheduler", runtime.GOOS)
+	}
+	runTest("timer_stop_reset_race.go", optionsFromTarget("", sema), t, nil, nil)
+}
+
 func runPlatTests(options compileopts.Options, tests []string, t *testing.T) {
 	emuCheck(t, options)
 

src/runtime/scheduler.go

@@ -52,6 +52,46 @@ func timerQueueRemove(t *timer) *timerNode {
 	return nil
 }
 
+// firingTimers is a list of timer nodes whose callback is currently running.
+// It is only used by schedulers that run timer callbacks concurrently with user
+// goroutines (the threads and cores schedulers), so that a timer stopped or
+// reset while its callback is running is not re-added to the timer queue by a
+// periodic timer's callback. Access is protected by the scheduler's timer lock.
+var firingTimers *timerNode
+
+// firingTimersAdd marks the given timer node as currently firing. The caller
+// must hold the scheduler's timer lock.
+func firingTimersAdd(tn *timerNode) {
+	tn.stopped = false
+	tn.firingNext = firingTimers
+	firingTimers = tn
+}
+
+// firingTimersRemove removes the given timer node from the firing list. The
+// caller must hold the scheduler's timer lock.
+func firingTimersRemove(tn *timerNode) {
+	for q := &firingTimers; *q != nil; q = &(*q).firingNext {
+		if *q == tn {
+			*q = tn.firingNext
+			tn.firingNext = nil
+			return
+		}
+	}
+}
+
+// firingTimerStop marks a currently-firing timer as stopped, so that its
+// callback will not re-add it to the queue. It returns whether the timer is
+// currently firing. The caller must hold the scheduler's timer lock.
+func firingTimerStop(tim *timer) bool {
+	for tn := firingTimers; tn != nil; tn = tn.firingNext {
+		if tn.timer == tim {
+			tn.stopped = true
+			return true
+		}
+	}
+	return false
+}
+
 // Goexit terminates the currently running goroutine. No other goroutines are affected.
 func Goexit() {
 	panicOrGoexit(nil, panicGoexit)

src/runtime/scheduler_cooperative.go

@@ -115,6 +115,15 @@ func addTimer(tim *timerNode) {
 	interrupt.Restore(mask)
 }
 
+// reAddTimer advances and re-adds a periodic timer (a ticker) after its
+// callback has run. The cooperative scheduler runs timer callbacks to
+// completion inside the scheduler loop, so a timer can't be stopped or reset
+// while its callback is running and the timer can always be re-added directly.
+func reAddTimer(tn *timerNode) {
+	tn.timer.when += tn.timer.period
+	addTimer(tn)
+}
+
 // removeTimer is the implementation of time.stopTimer. It removes a timer from
 // the timer queue, returning it if the timer is present in the timer queue.
 func removeTimer(tim *timer) *timerNode {

src/runtime/scheduler_cores.go

@@ -102,9 +102,38 @@ func addTimer(tn *timerNode) {
 	schedulerLock.Unlock()
 }
 
+// reAddTimer advances and re-adds a periodic timer (a ticker) after its
+// callback has run, unless it was stopped or reset while the callback was
+// running (in which case it must not be re-added).
+func reAddTimer(tn *timerNode) {
+	schedulerLock.Lock()
+
+	// Remove the timer from the firing list before re-adding it to the queue,
+	// so that another core popping it off the queue can't insert it into the
+	// firing list a second time (which would corrupt the list).
+	firingTimersRemove(tn)
+
+	if tn.stopped {
+		// The timer was stopped or reset while its callback was running. Don't
+		// re-add it: a stopped ticker must stay stopped, and a reset ticker has
+		// already been re-added by resetTimer.
+		schedulerLock.Unlock()
+		return
+	}
+	tn.timer.when += tn.timer.period
+	timerQueueAdd(tn)
+	interruptSleepTicksMulticore(tn.whenTicks())
+	schedulerLock.Unlock()
+}
+
 func removeTimer(t *timer) *timerNode {
 	schedulerLock.Lock()
 	n := timerQueueRemove(t)
+	if n == nil {
+		// The timer wasn't in the queue. It might be running its callback right
+		// now; if so, mark it stopped so it won't be re-added.
+		firingTimerStop(t)
+	}
 	schedulerLock.Unlock()
 	return n
 }
@@ -201,10 +230,21 @@ func scheduler(_ bool) {
 				timerQueue = tn.next
 				tn.next = nil
 
+				// Mark the timer as firing, so that a concurrent Stop or Reset
+				// (via removeTimer) can prevent a periodic timer from re-adding
+				// itself in its callback.
+				firingTimersAdd(tn)
+
 				// Run the callback stored in this timer node.
 				schedulerLock.Unlock()
 				tn.callback(tn, delay)
 				schedulerLock.Lock()
+				// A periodic timer (a ticker) already removed itself from the
+				// firing list in reAddTimer; a one-shot timer isn't re-added, so
+				// remove it from the firing list here.
+				if tn.timer.period == 0 {
+					firingTimersRemove(tn)
+				}
 				continue
 			}
 		}

src/runtime/scheduler_none.go

@@ -59,6 +59,10 @@ func addTimer(tim *timerNode) {
 	runtimePanic("timers not supported without a scheduler")
 }
 
+func reAddTimer(tn *timerNode) {
+	runtimePanic("timers not supported without a scheduler")
+}
+
 func removeTimer(tim *timer) *timerNode {
 	runtimePanic("timers not supported without a scheduler")
 	return nil

src/runtime/scheduler_threads.go

@@ -90,11 +90,25 @@ func timerRunner() {
 		timerQueue = tn.next
 		tn.next = nil
 
+		// Mark the timer as firing, so that a concurrent Stop or Reset (via
+		// removeTimer) can prevent a periodic timer from re-adding itself in its
+		// callback below.
+		firingTimersAdd(tn)
+
 		timerQueueLock.Unlock()
 
 		// Run the callback stored in this timer node.
 		delay := ticksToNanoseconds(now - tn.whenTicks())
 		tn.callback(tn, delay)
+
+		// The callback has finished running. A periodic timer (a ticker) already
+		// removed itself from the firing list in reAddTimer; a one-shot timer
+		// isn't re-added, so remove it from the firing list here.
+		timerQueueLock.Lock()
+		if tn.timer.period == 0 {
+			firingTimersRemove(tn)
+		}
+		timerQueueLock.Unlock()
 	}
 }
 
@@ -114,9 +128,42 @@ func addTimer(tim *timerNode) {
 	timerQueueLock.Unlock()
 }
 
+// reAddTimer advances and re-adds a periodic timer (a ticker) after its
+// callback has run, unless it was stopped or reset while the callback was
+// running (in which case it must not be re-added).
+func reAddTimer(tn *timerNode) {
+	timerQueueLock.Lock()
+
+	// Remove the timer from the firing list before re-adding it to the queue,
+	// so that another core popping it off the queue can't insert it into the
+	// firing list a second time (which would corrupt the list).
+	firingTimersRemove(tn)
+
+	if tn.stopped {
+		// The timer was stopped or reset while its callback was running. Don't
+		// re-add it: a stopped ticker must stay stopped, and a reset ticker has
+		// already been re-added by resetTimer.
+		timerQueueLock.Unlock()
+		return
+	}
+
+	tn.timer.when += tn.timer.period
+	timerQueueAdd(tn)
+
+	timerFutex.Add(1)
+	timerFutex.Wake()
+
+	timerQueueLock.Unlock()
+}
+
 func removeTimer(tim *timer) *timerNode {
 	timerQueueLock.Lock()
 	n := timerQueueRemove(tim)
+	if n == nil {
+		// The timer wasn't in the queue. It might be running its callback right
+		// now; if so, mark it stopped so it won't be re-added.
+		firingTimerStop(tim)
+	}
 	timerQueueLock.Unlock()
 	return n
 }

src/runtime/time.go

@@ -53,13 +53,13 @@ func stopTimer(tim *timeTimer) bool {
 
 //go:linkname resetTimer time.resetTimer
 func resetTimer(t *timeTimer, when, period int64) bool {
-	t.timer.when = when
-	t.timer.period = period
 	n := removeTimer(&t.timer)
 	removed := n != nil
 	if n == nil {
 		n = new(timerNode)
 	}
+	t.timer.when = when
+	t.timer.period = period
 	n.timer = &t.timer
 	n.callback = timerCallback
 	addTimer(n)
@@ -84,6 +84,19 @@ type timerNode struct {
 	next     *timerNode
 	timer    *timer
 	callback func(node *timerNode, delta int64)
+
+	// The following fields are only used by schedulers that run timer
+	// callbacks concurrently with user goroutines (the threads and cores
+	// schedulers). They make it possible to stop or reset a periodic timer (a
+	// ticker) while its callback is running, without the callback re-adding the
+	// timer to the queue afterwards. They are protected by the scheduler's
+	// timer lock.
+	//
+	// firingNext links nodes whose callback is currently running into the
+	// firingTimers list. stopped is set when the timer was stopped or reset
+	// while its callback was running, so that timerCallback does not re-add it.
+	firingNext *timerNode
+	stopped    bool
 }
 
 // whenTicks returns the (absolute) time when this timer should trigger next.
@@ -108,8 +121,7 @@ func timerCallback(tn *timerNode, delta int64) {
 
 	// If this is a periodic timer (a ticker), re-add it to the queue.
 	if tn.timer.period != 0 {
-		tn.timer.when += tn.timer.period
-		addTimer(tn)
+		reAddTimer(tn)
 	}
 }
 

testdata/timer_stop_reset_race.go

@@ -0,0 +1,83 @@
+package main
+
+import (
+	"time"
+	"unsafe"
+	_ "unsafe"
+)
+
+func main() {
+	testStopWhileFiring()
+	testResetWhileFiring()
+	println("timer stop/reset race tests done")
+}
+
+func testStopWhileFiring() {
+	started := make(chan struct{})
+	release := make(chan struct{})
+
+	tim := newTimerWithCallback(time.Millisecond, 0, func(any, uintptr, int64) {
+		close(started)
+		<-release
+	})
+
+	<-started
+	if stopTimer(tim) {
+		println("fail: Stop returned true for a firing timer")
+	} else {
+		println("Stop returned false for firing timer")
+	}
+	close(release)
+}
+
+func testResetWhileFiring() {
+	started := make(chan struct{})
+	release := make(chan struct{})
+	firedAgain := make(chan struct{}, 1)
+	first := true
+
+	tim := newTimerWithCallback(time.Millisecond, 5*time.Second, func(any, uintptr, int64) {
+		if first {
+			first = false
+			close(started)
+			<-release
+			return
+		}
+		select {
+		case firedAgain <- struct{}{}:
+		default:
+		}
+	})
+
+	<-started
+	resetPeriodicTimer(tim, 20*time.Millisecond, 5*time.Second)
+	close(release)
+	time.Sleep(200 * time.Millisecond)
+	select {
+	case <-firedAgain:
+		println("reset firing timer used reset deadline")
+	default:
+		println("fail: reset firing timer missed reset deadline")
+	}
+	stopTimer(tim)
+}
+
+func newTimerWithCallback(delay, period time.Duration, f func(any, uintptr, int64)) *time.Timer {
+	return newTimer(runtimeNano()+int64(delay), int64(period), f, nil, nil)
+}
+
+func resetPeriodicTimer(tim *time.Timer, delay, period time.Duration) bool {
+	return resetTimer(tim, runtimeNano()+int64(delay), int64(period))
+}
+
+//go:linkname newTimer time.newTimer
+func newTimer(when, period int64, f func(any, uintptr, int64), arg any, cp unsafe.Pointer) *time.Timer
+
+//go:linkname stopTimer time.stopTimer
+func stopTimer(tim *time.Timer) bool
+
+//go:linkname resetTimer time.resetTimer
+func resetTimer(tim *time.Timer, when, period int64) bool
+
+//go:linkname runtimeNano time.runtimeNano
+func runtimeNano() int64

testdata/timer_stop_reset_race.txt

@@ -0,0 +1,3 @@
+Stop returned false for firing timer
+reset firing timer used reset deadline
+timer stop/reset race tests done

testdata/timers.go

@@ -21,7 +21,11 @@ func main() {
 	<-ticker.C
 	println("waited on ticker at 1000ms")
 	ticker.Stop()
-	// Drain any tick that was already queued before Stop took effect.
+	// Ticker.Stop does not drain already-buffered ticks from the channel, and
+	// on a slow CI runner a final tick may be delivered concurrently right as
+	// Stop is called. Give any such in-flight tick a chance to arrive, then
+	// drain the channel before checking that no further ticks are sent.
+	time.Sleep(time.Millisecond * 100)
 	select {
 	case <-ticker.C:
 	default: