From 336617186a6c01a1874685d4577042ab007609ea Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 29 Feb 2024 11:56:07 -0500 Subject: [PATCH] time: make sure tests avoid the special-case channel code Many of the tests in package time are about proper manipulation of the timer heap. But now NewTimer bypasses the timer heap except when something is blocked on the associated channel. Make the tests test the heap again by using AfterFunc instead of NewTimer. In particular, adds a non-chan version of TestZeroTimer, which was flaky-broken and then fixed by CLs in the cleanup stack. This new tests makes sure we notice if it breaks again. Fixes #66006. Change-Id: Ib59fc1b8b85ef5a21e72fe418c627c9b8b8a083a Reviewed-on: https://go-review.googlesource.com/c/go/+/568255 LUCI-TryBot-Result: Go LUCI Reviewed-by: Ian Lance Taylor Auto-Submit: Russ Cox Reviewed-by: Austin Clements --- src/time/sleep_test.go | 95 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 18 deletions(-) diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go index e985870710..3339799f1d 100644 --- a/src/time/sleep_test.go +++ b/src/time/sleep_test.go @@ -18,6 +18,17 @@ import ( _ "unsafe" // for go:linkname ) +// newTimerFunc simulates NewTimer using AfterFunc, +// but this version will not hit the special cases for channels +// that are used when calling NewTimer. +// This makes it easy to test both paths. +func newTimerFunc(d Duration) *Timer { + c := make(chan Time, 1) + t := AfterFunc(d, func() { c <- Now() }) + t.C = c + return t +} + // haveHighResSleep is true if the system supports at least ~1ms sleeps. // //go:linkname haveHighResSleep runtime.haveHighResSleep @@ -58,7 +69,7 @@ func TestSleep(t *testing.T) { } } -// Test the basic function calling behavior. Correct queueing +// Test the basic function calling behavior. Correct queuing // behavior is tested elsewhere, since After and AfterFunc share // the same code. func TestAfterFunc(t *testing.T) { @@ -198,10 +209,19 @@ func BenchmarkAfter(b *testing.B) { } func BenchmarkStop(b *testing.B) { - benchmark(b, func(n int) { - for i := 0; i < n; i++ { - NewTimer(1 * Second).Stop() - } + b.Run("impl=chan", func(b *testing.B) { + benchmark(b, func(n int) { + for i := 0; i < n; i++ { + NewTimer(1 * Second).Stop() + } + }) + }) + b.Run("impl=func", func(b *testing.B) { + benchmark(b, func(n int) { + for i := 0; i < n; i++ { + newTimerFunc(1 * Second).Stop() + } + }) }) } @@ -230,12 +250,23 @@ func BenchmarkStartStop(b *testing.B) { } func BenchmarkReset(b *testing.B) { - benchmark(b, func(n int) { - t := NewTimer(Hour) - for i := 0; i < n; i++ { - t.Reset(Hour) - } - t.Stop() + b.Run("impl=chan", func(b *testing.B) { + benchmark(b, func(n int) { + t := NewTimer(Hour) + for i := 0; i < n; i++ { + t.Reset(Hour) + } + t.Stop() + }) + }) + b.Run("impl=chan", func(b *testing.B) { + benchmark(b, func(n int) { + t := newTimerFunc(Hour) + for i := 0; i < n; i++ { + t.Reset(Hour) + } + t.Stop() + }) }) } @@ -288,6 +319,15 @@ func TestAfterTick(t *testing.T) { } func TestAfterStop(t *testing.T) { + t.Run("impl=chan", func(t *testing.T) { + testAfterStop(t, NewTimer) + }) + t.Run("impl=func", func(t *testing.T) { + testAfterStop(t, newTimerFunc) + }) +} + +func testAfterStop(t *testing.T, newTimer func(Duration) *Timer) { // We want to test that we stop a timer before it runs. // We also want to test that it didn't run after a longer timer. // Since we don't want the test to run for too long, we don't @@ -303,7 +343,7 @@ func TestAfterStop(t *testing.T) { for i := 0; i < 5; i++ { AfterFunc(100*Millisecond, func() {}) - t0 := NewTimer(50 * Millisecond) + t0 := newTimer(50 * Millisecond) c1 := make(chan bool, 1) t1 := AfterFunc(150*Millisecond, func() { c1 <- true }) c2 := After(200 * Millisecond) @@ -344,13 +384,22 @@ func TestAfterStop(t *testing.T) { } func TestAfterQueuing(t *testing.T) { + t.Run("impl=chan", func(t *testing.T) { + testAfterQueuing(t, After) + }) + t.Run("impl=func", func(t *testing.T) { + testAfterQueuing(t, func(d Duration) <-chan Time { return newTimerFunc(d).C }) + }) +} + +func testAfterQueuing(t *testing.T, after func(Duration) <-chan Time) { // This test flakes out on some systems, // so we'll try it a few times before declaring it a failure. const attempts = 5 err := errors.New("!=nil") for i := 0; i < attempts && err != nil; i++ { delta := Duration(20+i*50) * Millisecond - if err = testAfterQueuing(delta); err != nil { + if err = testAfterQueuing1(delta, after); err != nil { t.Logf("attempt %v failed: %v", i, err) } } @@ -370,9 +419,9 @@ func await(slot int, result chan<- afterResult, ac <-chan Time) { result <- afterResult{slot, <-ac} } -func testAfterQueuing(delta Duration) error { +func testAfterQueuing1(delta Duration, after func(Duration) <-chan Time) error { // make the result channel buffered because we don't want - // to depend on channel queueing semantics that might + // to depend on channel queuing semantics that might // possibly change in the future. result := make(chan afterResult, len(slots)) @@ -566,13 +615,22 @@ func TestZeroTimerStopPanics(t *testing.T) { // Test that zero duration timers aren't missed by the scheduler. Regression test for issue 44868. func TestZeroTimer(t *testing.T) { + t.Run("impl=chan", func(t *testing.T) { + testZeroTimer(t, NewTimer) + }) + t.Run("impl=func", func(t *testing.T) { + testZeroTimer(t, newTimerFunc) + }) +} + +func testZeroTimer(t *testing.T, newTimer func(Duration) *Timer) { if testing.Short() { t.Skip("-short") } for i := 0; i < 1000000; i++ { s := Now() - ti := NewTimer(0) + ti := newTimer(0) <-ti.C if diff := Since(s); diff > 2*Second { t.Errorf("Expected time to get value from Timer channel in less than 2 sec, took %v", diff) @@ -591,7 +649,7 @@ func TestTimerModifiedEarlier(t *testing.T) { count := 1000 fail := 0 for i := 0; i < count; i++ { - timer := NewTimer(Hour) + timer := newTimerFunc(Hour) for j := 0; j < 10; j++ { if !timer.Stop() { <-timer.C @@ -638,7 +696,8 @@ func TestAdjustTimers(t *testing.T) { switch state { case 0: - timers[i] = NewTimer(0) + timers[i] = newTimerFunc(0) + case 1: <-timer.C // Timer is now idle. -- 2.48.1