diff --git a/mock.go b/mock.go index 7255aff..6325cd8 100644 --- a/mock.go +++ b/mock.go @@ -30,6 +30,9 @@ type Mock struct { logger Logger mu sync.Mutex testOver bool + // writes holds the timers that have a goroutine sending their fired value on C. The cleanup + // registered in NewMock ends those goroutines. + writes map[*Timer]struct{} // cur is the current time cur time.Time @@ -68,9 +71,10 @@ func (m *Mock) TickerFunc(ctx context.Context, d time.Duration, f func() error, return t } -// NewTicker creates a mocked ticker attached to this Mock. Note that it will cease sending ticks on its channel at the -// end of the test, to avoid leaking any goroutines. Ticks are suppressed even if the mock clock is advanced after the -// test completes. Best practice is to only manipulate the mock time in the main goroutine of the test. +// NewTicker creates a mocked ticker attached to this Mock. The ticker stops sending ticks when the +// cleanup that NewTicker registers runs, and advancing the clock after that sends none. A Reset +// after that cleanup starts the ticker again, and only Stop ends it then. Best practice is to only +// manipulate the mock time in the main goroutine of the test. func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker { if d <= 0 { panic("NewTicker called with negative or zero duration") @@ -99,6 +103,7 @@ func (m *Mock) NewTimer(d time.Duration, tags ...string) *Timer { if d <= 0 { // zero or negative duration timer means we should immediately fire // it, rather than add it. + t.nxt = m.cur go t.fire(t.mock.cur) return t } @@ -120,6 +125,7 @@ func (m *Mock) AfterFunc(d time.Duration, f func(), tags ...string) *Timer { if d <= 0 { // zero or negative duration timer means we should immediately fire // it, rather than add it. + t.nxt = m.cur go t.fire(t.mock.cur) return t } @@ -480,6 +486,13 @@ func (m *Mock) WithLogger(l Logger) *Mock { // NewMock creates a new Mock with the time set to midnight UTC on Jan 1, 2024. // You may re-set the time earlier than this, but only before timers or tickers // are created. +// +// Cleanups registered after NewMock run before NewMock's own cleanup, so they can still receive +// the value of a timer that fired during the test. NewMock's cleanup drops timer values that have +// not been received, and a timer from NewTimer that fires after it sends only to a reader already +// waiting on C. Call NewMock before registering cleanups that wait for code reading the Mock's +// timers. Ticks from NewTicker stop earlier, unless Reset starts the ticker again; see +// [Mock.NewTicker]. func NewMock(tb TestingT) *Mock { cur, err := time.Parse(time.RFC3339, "2024-01-01T00:00:00Z") if err != nil { @@ -489,11 +502,15 @@ func NewMock(tb TestingT) *Mock { tb: tb, logger: tb, cur: cur, + writes: make(map[*Timer]struct{}), } tb.Cleanup(func() { m.mu.Lock() defer m.mu.Unlock() m.testOver = true + for t := range m.writes { + t.endWriteLocked() + } m.logger.Logf("Mock Clock - test cleanup; will no longer log clock events") }) return m diff --git a/mock_test.go b/mock_test.go index 7bc822d..5c7ac26 100644 --- a/mock_test.go +++ b/mock_test.go @@ -9,6 +9,7 @@ import ( "runtime/pprof" "strings" "testing" + "testing/synctest" "time" "github.com/coder/quartz" @@ -508,46 +509,105 @@ func (l *testLogger) Logf(format string, args ...any) { l.calls = append(l.calls, fmt.Sprintf(format, args...)) } -func TestTimerStop_Go123(t *testing.T) { +// TestTimer_StopAndReset checks what Stop and Reset return for a timer that has fired, and that +// no value is received after they return and Reset's schedule stays, as with a time.Timer. +func TestTimer_StopAndReset(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - - tmr := mClock.NewTimer(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) - if tmr.Stop() { - t.Fatal("timer hadn't already been active") - } - - select { - case tme := <-tmr.C: - t.Fatalf("got channel read after stop: %s", tme) - default: - // OK! - } -} - -func TestTimerReset_Go123(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - mClock := quartz.NewMock(t) - - tmr := mClock.NewTimer(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) - if tmr.Reset(1 * time.Second) { - t.Fatal("timer hadn't already been active") + fired := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + tmr := m.NewTimer(time.Second) + m.Advance(time.Second).MustWait(ctx) + return tmr + } + received := func(ctx context.Context, m *quartz.Mock) *quartz.Timer { + tmr := fired(ctx, m) + <-tmr.C + return tmr + } + // NewTimer(0) fires from a new goroutine, which can run before or after the call. + zeroDuration := func(_ context.Context, m *quartz.Mock) *quartz.Timer { + return m.NewTimer(0) + } + // Advance fires the timer from a new goroutine, which can run before or after the call. + advancing := func(_ context.Context, m *quartz.Mock) *quartz.Timer { + tmr := m.NewTimer(time.Second) + _ = m.Advance(time.Second) + return tmr + } + stop := func(tmr *quartz.Timer) bool { return tmr.Stop() } + reset := func(tmr *quartz.Timer) bool { return tmr.Reset(time.Hour) } + for _, tc := range []struct { + name string + start func(context.Context, *quartz.Mock) *quartz.Timer + call func(*quartz.Timer) bool + want bool + // wantNext is how long after the call the timer fires, or 0 if it does not. + wantNext time.Duration + }{ + // Stop returning true here keeps if !t.Stop() { <-t.C } from blocking. + {name: "StopFiredUnread", start: fired, call: stop, want: true}, + {name: "StopFiredReceived", start: received, call: stop, want: false}, + {name: "ResetFiredUnread", start: fired, call: reset, want: true, wantNext: time.Hour}, + {name: "StopZeroDuration", start: zeroDuration, call: stop, want: true}, + { + name: "ResetZeroDuration", start: zeroDuration, call: reset, + want: true, wantNext: time.Hour, + }, + {name: "StopDuringAdvance", start: advancing, call: stop, want: true}, + { + name: "ResetDuringAdvance", start: advancing, call: reset, + want: true, wantNext: time.Hour, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + tmr := tc.start(ctx, mClock) + if got := tc.call(tmr); got != tc.want { + t.Errorf("got %v, want %v", got, tc.want) + } + // Lets any goroutine that could still send on C run before the checks. + synctest.Wait() + select { + case tme := <-tmr.C: + t.Errorf("received %s after the call returned", tme) + default: + } + if next, _ := mClock.Peek(); next != tc.wantNext { + t.Errorf("timer fires in %s, want %s", next, tc.wantNext) + } + }) + }) } - select { - case tme := <-tmr.C: - t.Fatalf("got channel read after stop: %s", tme) - default: - // OK! - } + // A time.AfterFunc timer's Stop returns true if f never runs, and false if f has started. + // AfterFunc(0) runs f from a new goroutine, and Stop usually gets the Mock's lock before it: + // the goroutine gets it first in a few in ten thousand runs with -race. No Mock call can hold + // that goroutine back, so this accepts either order and checks that Stop's result agrees with + // whether f ran. + t.Run("StopAfterFuncZeroDuration", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ran := make(chan struct{}) + tmr := quartz.NewMock(t).AfterFunc(0, func() { close(ran) }) + stopped := tmr.Stop() + // Lets f's goroutine run before the check. + synctest.Wait() + select { + case <-ran: + if stopped { + t.Error("f ran, but Stop returned true") + } + default: + if !stopped { + t.Error("f has not run, but Stop returned false") + } + } + }) + }) } // TestTimerReset_NegativeDuration verifies that resetting a still-active timer @@ -591,16 +651,142 @@ func TestTimerReset_NegativeDuration(t *testing.T) { } } +// TestTimer_ReadableDuringCleanup checks that a cleanup registered after NewMock, which runs before +// NewMock's cleanup, can still receive from a timer that fired during the test. Tests that stop a +// server in such a cleanup rely on this when a worker reads the timer on its way out. +func TestTimer_ReadableDuringCleanup(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + mClock := quartz.NewMock(t) + busy := make(chan struct{}) + quit := make(chan struct{}) + exited := make(chan struct{}) + // Stands in for a server's Close: it unblocks the worker and waits for it to exit. Bubble + // time moves once every goroutine is blocked, so a worker that never receives fails the + // test at once. quit then lets the worker exit, so the bubble can end. + t.Cleanup(func() { + close(busy) + select { + case <-exited: + case <-time.After(10 * time.Second): + t.Error("worker did not receive the timer value during cleanup") + } + close(quit) + }) + tmr := mClock.NewTimer(time.Second) + go func() { + defer close(exited) + <-busy + select { + case <-tmr.C: + case <-quit: + } + }() + mClock.Advance(time.Second).MustWait(ctx) + }) +} + +// TestTimer_ReaderWaitingAtMockCleanup checks that a reader waiting on C when NewMock's cleanup +// runs receives the value of a timer that fires just before or after that cleanup. synctest.Wait +// makes sure the reader is waiting before the timer fires and that the send has ended before the +// check. +func TestTimer_ReaderWaitingAtMockCleanup(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fireAfterCleanup bool + }{ + // The test fires the timer as its last step. In a bubble, the goroutine that sends the + // value often starts only after NewMock's cleanup is waiting to end it. + {name: "FiredBeforeCleanup"}, + {name: "FiredAfterCleanup", fireAfterCleanup: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // When the sending goroutine starts late, select picks at random between the reader + // and the cleanup, so a send that does not try the reader first loses the value about + // half the time. Each round is a new bubble, so without the race detector, which + // changes scheduling, the test then fails in nearly every run. + for range 10 { + synctest.Test(t, func(t *testing.T) { + var mClock *quartz.Mock + received := make(chan struct{}, 1) + quit := make(chan struct{}) + // Registered before NewMock, so it runs after NewMock's cleanup. + t.Cleanup(func() { + if tc.fireAfterCleanup { + mClock.Advance(time.Second) + } + synctest.Wait() + close(quit) + if len(received) == 0 { + t.Error("the waiting reader did not receive the timer value") + } + }) + mClock = quartz.NewMock(t) + tmr := mClock.NewTimer(time.Second) + go func() { + select { + case <-tmr.C: + received <- struct{}{} + case <-quit: + } + }() + synctest.Wait() + if !tc.fireAfterCleanup { + mClock.Advance(time.Second).MustWait(context.Background()) + } + }) + } + }) + } +} + +// TestNoLeak_Go123 relies on verifyNoLeakTestMain to report goroutines that outlive the tests. func TestNoLeak_Go123(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - mClock := quartz.NewMock(t) + t.Run("FiredDuringTest", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - _ = mClock.NewTimer(1 * time.Second) - _ = mClock.NewTicker(1 * time.Second) - mClock.Advance(1 * time.Second).MustWait(ctx) + mClock := quartz.NewMock(t) + + _ = mClock.NewTimer(1 * time.Second) + _ = mClock.NewTicker(1 * time.Second) + mClock.Advance(1 * time.Second).MustWait(ctx) + }) + + // NewTimer(0) and Reset(0) each fire the timer from a new goroutine, and in most of 100 rounds + // both run after Reset. Only one of them may start a write: the Mock tracks one write per + // timer, so its cleanup would not end a second one. + t.Run("ZeroDurationTimerResetToZero", func(t *testing.T) { + t.Parallel() + for range 100 { + tmr := quartz.NewMock(t).WithLogger(quartz.NoOpLogger).NewTimer(0) + tmr.Reset(0) + } + }) + + // A test that stops waiting on an Advance can end before its timers fire. + t.Run("TimerFiredAfterTestEnds", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + var mClock *quartz.Mock + t.Run("Inner", func(t *testing.T) { + mClock = quartz.NewMock(t) + _ = mClock.NewTimer(time.Second) + }) + if err := mClock.Advance(time.Second).Wait(ctx); err != nil { + t.Fatal(err) + } + }) } func TestTickerStop_Go123(t *testing.T) { diff --git a/timer.go b/timer.go index 0f5f733..498463d 100644 --- a/timer.go +++ b/timer.go @@ -17,52 +17,98 @@ type Timer struct { fn func() // AfterFunc function, if set stopped bool // True if stopped, false if running - // As of Go 1.23, timer channels are unbuffered and guaranteed to block forever after a call to stop. + // As of Go 1.23, timer channels are unbuffered and guaranteed to block forever after a call to + // stop. // - // When a mocked timer fires, we don't want to block on a channel write, because it's fine for the code under test - // not to be reading. That means we need to start a new goroutine to do the channel write if we are a channel-based - // timer. + // When a mocked timer fires, we don't want to block on a channel write, because it's fine for + // the code under test not to be reading. That means we need to start a new goroutine to do the + // channel write if we are a channel-based timer. // - // They also are not supposed to leak even if they are never read or stopped (Go runtime can garbage collect them). - // We can't garbage-collect because we can't check if any other code besides the mock references, but we can ensure - // that we don't leak goroutines so that the garbage collector can do its job when the mock is no longer - // referenced. The channels below allow us to interrupt the channel write goroutine. + // They also are not supposed to leak even if they are never read or stopped (Go runtime can + // garbage collect them). We can't garbage-collect because we can't check if any other code + // besides the mock references, but we can ensure that we don't leak goroutines so that the + // garbage collector can do its job when the mock is no longer referenced. interrupt lets Stop, + // Reset and the cleanup registered by NewMock end the write goroutine. interrupt chan struct{} } func (t *Timer) fire(tt time.Time) { t.mock.mu.Lock() + // Drop a fire whose schedule is gone: the timer was stopped, has already fired, or Reset + // moved nxt. Fires for a non-positive duration and fires from Advance run on new goroutines, + // so Stop, Reset or an earlier fire can take the lock first. + if t.stopped || !t.nxt.Equal(tt) { + t.mock.mu.Unlock() + return + } t.mock.removeTimerLocked(t) if t.fn != nil { t.mock.mu.Unlock() t.fn() return + } else if t.mock.testOver { + t.mock.mu.Unlock() + // NewMock's cleanup has run and would not end a write goroutine, so only a reader that is + // already waiting gets the value. + select { + case t.c <- tt: + default: + } } else { interrupt := make(chan struct{}) - // Prevents the goroutine from leaking beyond the test. Side effect is that timer channels cannot be read - // after the test exits. - t.mock.tb.Cleanup(func() { - <-interrupt - }) t.interrupt = interrupt + t.mock.writes[t] = struct{}{} t.mock.mu.Unlock() go func() { - defer close(interrupt) + // select picks at random among ready cases. Send first to a reader that is already + // waiting, so it gets the value even when Stop, Reset or NewMock's cleanup is waiting + // on interrupt. select { case t.c <- tt: - case interrupt <- struct{}{}: + default: + select { + case t.c <- tt: + case interrupt <- struct{}{}: + // endWriteLocked received this and removed the write. + return + } + } + // Close before locking: endWriteLocked waits on interrupt with the lock held. + close(interrupt) + t.mock.mu.Lock() + defer t.mock.mu.Unlock() + // If interrupt has changed, endWriteLocked has removed this write, and Reset may have + // started another. + if t.interrupt == interrupt { + t.interrupt = nil + delete(t.mock.writes, t) } }() } } +// endWriteLocked ends the goroutine that fire started to send on C, if it has not ended yet. It +// reports whether that goroutine ended without sending the value. +func (t *Timer) endWriteLocked() bool { + if t.interrupt == nil { + return false + } + // The goroutine sends on interrupt when it ends without sending the value, and closes + // interrupt once it has sent the value. + _, unsent := <-t.interrupt + t.interrupt = nil + delete(t.mock.writes, t) + return unsent +} + func (t *Timer) next() time.Time { return t.nxt } // Stop prevents the Timer from firing. It returns true if the call stops the timer, false if the -// timer has already expired or been stopped. Stop does not close the channel, to prevent a read -// from the channel succeeding incorrectly. +// timer has already expired or been stopped. As with a time.Timer, a timer whose value has not been +// received has not expired: Stop discards the value and returns true. Stop does not close the +// channel, to prevent a read from the channel succeeding incorrectly. // // See https://pkg.go.dev/time#Timer.Stop for more information. func (t *Timer) Stop(tags ...string) bool { @@ -76,16 +122,15 @@ func (t *Timer) Stop(tags ...string) bool { defer close(c.complete) result := !t.stopped t.mock.removeTimerLocked(t) - // check if we've already fired, and if so, interrupt it. - if t.interrupt != nil { - <-t.interrupt - t.interrupt = nil + if t.endWriteLocked() { + result = true } return result } // Reset changes the timer to expire after duration d. It returns true if the timer had been active, -// false if the timer had expired or been stopped. +// false if the timer had expired or been stopped. As with a time.Timer, a timer whose value has not +// been received is active: Reset discards the value and returns true. // // See https://pkg.go.dev/time#Timer.Reset for more information. func (t *Timer) Reset(d time.Duration, tags ...string) bool { @@ -98,10 +143,8 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool { t.mock.matchCallLocked(c) defer close(c.complete) result := !t.stopped - // check if we've already fired, and if so, interrupt it. - if t.interrupt != nil { - <-t.interrupt - t.interrupt = nil + if t.endWriteLocked() { + result = true } // Remove the timer from the mock's event list synchronously; otherwise the // stale old schedule can cause e.g. Advance() to reject advancing past the @@ -111,6 +154,7 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool { // zero or negative duration timer means we should immediately re-fire // it, rather than re-add it. t.stopped = false + t.nxt = t.mock.cur go t.fire(t.mock.cur) return result }