runtime: don't frob isSending for tickers

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
index 3353502fc4df16a5f044971e5a38fb5f03ef3ab0..af19a6435df35954ba822ce8c3f83d5ca9908884 100644 (file)
--- 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
index 5357ed23c8e352ef92173680d6ed34e3a04bb72f..520ff957d09fc1c4f19d9adfa1a69613b1bc33f4 100644 (file)
--- 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