runtime: don't frob isSending for tickers

ianlancetaylor · gopherbot · commit 48849e0866f6 · 2024-10-14T19:04:43.000Z
The Ticker Stop and Reset methods don't report a value, so we don't need to track whether they are interrupting a send. This includes a test that used to fail about 2% of the time on my laptop when run under x/tools/cmd/stress. Change-Id: Ic6d14b344594149dd3c24b37bbe4e42e83f9a9ad Reviewed-on: https://go-review.googlesource.com/c/go/+/620136 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Michael Knyszek <mknyszek@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
diff --git a/src/runtime/time.go b/src/runtime/time.go
@@ -33,6 +33,7 @@ type timer struct {
 	// isSending is used to handle races between running a
 	// channel timer and stopping or resetting the timer.
 	// It is used only for channel timers (t.isChan == true).
+	// It is not used for tickers.
 	// The lowest zero bit is set when about to send a value on the channel,
 	// and cleared after sending the value.
 	// The stop/reset code uses this to detect whether it
@@ -467,7 +468,7 @@ func (t *timer) stop() bool {
 		// send from actually happening. That means
 		// that we should return true: the timer was
 		// stopped, even though t.when may be zero.
-		if t.isSending.Load() > 0 {
+		if t.period == 0 && t.isSending.Load() > 0 {
 			pending = true
 		}
 	}
@@ -529,6 +530,7 @@ func (t *timer) modify(when, period int64, f func(arg any, seq uintptr, delay in
 		t.maybeRunAsync()
 	}
 	t.trace("modify")
+	oldPeriod := t.period
 	t.period = period
 	if f != nil {
 		t.f = f
@@ -570,7 +572,7 @@ func (t *timer) modify(when, period int64, f func(arg any, seq uintptr, delay in
 		// send from actually happening. That means
 		// that we should return true: the timer was
 		// stopped, even though t.when may be zero.
-		if t.isSending.Load() > 0 {
+		if oldPeriod == 0 && t.isSending.Load() > 0 {
 			pending = true
 		}
 	}
@@ -1064,7 +1066,7 @@ func (t *timer) unlockAndRun(now int64) {
 
 	async := debug.asynctimerchan.Load() != 0
 	var isSendingClear uint8
-	if !async && t.isChan {
+	if !async && t.isChan && t.period == 0 {
 		// Tell Stop/Reset that we are sending a value.
 		// Set the lowest zero bit.
 		// We do this awkward step because atomic.Uint8
@@ -1115,9 +1117,12 @@ func (t *timer) unlockAndRun(now int64) {
 		// true meaning that no value was sent.
 		lock(&t.sendLock)
 
-		// We are committed to possibly sending a value based on seq,
-		// so no need to keep telling stop/modify that we are sending.
-		t.isSending.And(^isSendingClear)
+		if t.period == 0 {
+			// We are committed to possibly sending a value
+			// based on seq, so no need to keep telling
+			// stop/modify that we are sending.
+			t.isSending.And(^isSendingClear)
+		}
 
 		if t.seq != seq {
 			f = func(any, uintptr, int64) {}
diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go
@@ -847,6 +847,31 @@ func testStopResetResultGODEBUG(t *testing.T, testStop bool, godebug string) {
 	wg.Wait()
 }
 
+// Test having a large number of goroutines wake up a timer simultaneously.
+// This used to trigger a crash when run under x/tools/cmd/stress.
+func TestMultiWakeup(t *testing.T) {
+	if testing.Short() {
+		t.Skip("-short")
+	}
+
+	goroutines := runtime.GOMAXPROCS(0)
+	timer := NewTicker(Microsecond)
+	var wg sync.WaitGroup
+	wg.Add(goroutines)
+	for range goroutines {
+		go func() {
+			defer wg.Done()
+			for range 100000 {
+				select {
+				case <-timer.C:
+				case <-After(Millisecond):
+				}
+			}
+		}()
+	}
+	wg.Wait()
+}
+
 // Benchmark timer latency when the thread that creates the timer is busy with
 // other work and the timers must be serviced by other threads.
 // https://golang.org/issue/38860
