From ed8cbbc3ae96aef98d8f9e9e7003a99ed74992b5 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 21 Jul 2021 19:57:56 -0700 Subject: [PATCH] [release-branch.go1.16] runtime: don't clear timerModifiedEarliest if adjustTimers is 0 This avoids a race when a new timerModifiedEarlier timer is created by a different goroutine. For #47329 Fixes #47332 Change-Id: I6f6c87b4a9b5491b201c725c10bc98e23e0ed9d1 Reviewed-on: https://go-review.googlesource.com/c/go/+/336432 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Michael Knyszek (cherry picked from commit 798ec73519a7226d6d436e42498a54aed23b8468) Reviewed-on: https://go-review.googlesource.com/c/go/+/336689 --- src/runtime/runtime2.go | 2 +- src/runtime/time.go | 5 ----- src/time/sleep_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 9a032d8658c70..dd375da1936c5 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -653,7 +653,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 dee6a674e4cdd..7b84d2af57151 100644 --- 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 6ee0631a85564..e0172bf5e0b73 100644 --- 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