runtime: don't clear timerModifiedEarliest if adjustTimers is 0

This avoids a race when a new timerModifiedEarlier timer is created by
a different goroutine.

Fixes #47329

Change-Id: I6f6c87b4a9b5491b201c725c10bc98e23e0ed9d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/336432
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 2a66826f34bb8d1b0af42326dc93276380339b59..8a15787382c61017a30f99fcf6c77e8678540cbc 100644 (file)
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -681,7 +681,7 @@ type p struct {
        // timerModifiedEarlier status. Because the timer may have been
        // modified again, there need not be any timer with this value.
        // This is updated using atomic functions.
-       // This is 0 if the value is unknown.
+       // This is 0 if there are no timerModifiedEarlier timers.
        timerModifiedEarliest uint64
 
        // Per-P GC state

diff --git a/src/runtime/time.go b/src/runtime/time.go
index dee6a674e4cdd277a9924abe27dfd1a389ba5759..7b84d2af571515e6e37d442ec57fbeba2a06321b 100644 (file)
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -668,11 +668,6 @@ func adjusttimers(pp *p, now int64) {
                if verifyTimers {
                        verifyTimerHeap(pp)
                }
-               // There are no timers to adjust, so it is safe to clear
-               // timerModifiedEarliest. Do so in case it is stale.
-               // Everything will work if we don't do this,
-               // but clearing here may save future calls to adjusttimers.
-               atomic.Store64(&pp.timerModifiedEarliest, 0)
                return
        }
 

diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go
index 6ee0631a85564680bf91df0bc8e9d0ab0f3437a2..e0172bf5e0b7361891b6fcbef8c5db35460af763 100644 (file)
--- a/src/time/sleep_test.go
+++ b/src/time/sleep_test.go
@@ -527,6 +527,40 @@ func TestZeroTimer(t *testing.T) {
        }
 }
 
+// Test that rapidly moving a timer earlier doesn't cause it to get dropped.
+// Issue 47329.
+func TestTimerModifiedEarlier(t *testing.T) {
+       past := Until(Unix(0, 0))
+       count := 1000
+       fail := 0
+       for i := 0; i < count; i++ {
+               timer := NewTimer(Hour)
+               for j := 0; j < 10; j++ {
+                       if !timer.Stop() {
+                               <-timer.C
+                       }
+                       timer.Reset(past)
+               }
+
+               deadline := NewTimer(10 * Second)
+               defer deadline.Stop()
+               now := Now()
+               select {
+               case <-timer.C:
+                       if since := Since(now); since > 8*Second {
+                               t.Errorf("timer took too long (%v)", since)
+                               fail++
+                       }
+               case <-deadline.C:
+                       t.Error("deadline expired")
+               }
+       }
+
+       if fail > 0 {
+               t.Errorf("%d failures", fail)
+       }
+}
+
 // 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