fix: support Go 1.23+ timer & ticker channel behavior (#23)

fixes #22

Changes mock ticker and timer implementations based on channels to match Go 1.23+. c.f. https://go.dev/wiki/Go123Timer

1. Channels for tickers and timers are now unbuffered.
2. Channels reads will block after `Stop()` returns.
3. We use goroutines to write to the unbuffered channels. In order to avoid leaking go routines, when the test ends, any uncompleted channel writes are abandoned. This means that reads from the channels will block after the test exits. This is usually what you want, but is a small difference in behavior from before, where we wrote to a _buffered_ channel, and so reads could complete after the test completed.

Author: spikecurtis

.github/ci.yaml

@@ -7,6 +7,6 @@ jobs:
       - uses: actions/checkout@v2
       - uses: actions/setup-go@v4
         with:
-          go-version: "^1.21"
+          go-version: "^1.23"
       - name: test
         run: go test .

go.mod

@@ -1,3 +1,3 @@
 module github.com/coder/quartz
 
-go 1.21.8
+go 1.23.9

mock.go

@@ -55,6 +55,9 @@ 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.
 func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker {
 	if d <= 0 {
 		panic("NewTicker called with negative or zero duration")
@@ -64,17 +67,7 @@ func (m *Mock) NewTicker(d time.Duration, tags ...string) *Ticker {
 	c := newCall(clockFunctionNewTicker, tags, withDuration(d))
 	m.matchCallLocked(c)
 	defer close(c.complete)
-	// 1 element buffer follows standard library implementation
-	ticks := make(chan time.Time, 1)
-	t := &Ticker{
-		C:    ticks,
-		c:    ticks,
-		d:    d,
-		nxt:  m.cur.Add(d),
-		mock: m,
-	}
-	m.addEventLocked(t)
-	return t
+	return newMockTickerLocked(m, d)
 }
 
 func (m *Mock) NewTimer(d time.Duration, tags ...string) *Timer {
@@ -83,7 +76,7 @@ func (m *Mock) NewTimer(d time.Duration, tags ...string) *Timer {
 	c := newCall(clockFunctionNewTimer, tags, withDuration(d))
 	defer close(c.complete)
 	m.matchCallLocked(c)
-	ch := make(chan time.Time, 1)
+	ch := make(chan time.Time)
 	t := &Timer{
 		C:    ch,
 		c:    ch,
@@ -277,8 +270,8 @@ func (m *Mock) Advance(d time.Duration) AdvanceWaiter {
 		return w
 	}
 	if fin.After(m.nextTime) {
-		m.tb.Errorf(fmt.Sprintf("cannot advance %s which is beyond next timer/ticker event in %s",
-			d.String(), m.nextTime.Sub(m.cur)))
+		m.tb.Errorf("cannot advance %s which is beyond next timer/ticker event in %s",
+			d.String(), m.nextTime.Sub(m.cur))
 		m.mu.Unlock()
 		close(w.ch)
 		return w

mock_test.go

@@ -380,9 +380,8 @@ func Test_MultipleTrapsDeadlock(t *testing.T) {
 		trap1 := mClock.Trap().Now("1")
 		defer trap1.Close()
 
-		timeCh := make(chan time.Time)
 		go func() {
-			timeCh <- mClock.Now("0", "1")
+			mClock.Now("0", "1")
 		}()
 
 		c0 := trap0.MustWait(testCtx)
@@ -501,3 +500,76 @@ func (l *testLogger) Log(args ...any) {
 func (l *testLogger) Logf(format string, args ...any) {
 	l.calls = append(l.calls, fmt.Sprintf(format, args...))
 }
+
+func TestTimerStop_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.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")
+	}
+
+	select {
+	case tme := <-tmr.C:
+		t.Fatalf("got channel read after stop: %s", tme)
+	default:
+		// OK!
+	}
+}
+
+func TestNoLeak_Go123(t *testing.T) {
+	t.Parallel()
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	mClock := quartz.NewMock(t)
+
+	_ = mClock.NewTimer(1 * time.Second)
+	_ = mClock.NewTicker(1 * time.Second)
+	mClock.Advance(1 * time.Second).MustWait(ctx)
+}
+
+func TestTickerStop_Go123(t *testing.T) {
+	t.Parallel()
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	mClock := quartz.NewMock(t)
+
+	tkr := mClock.NewTicker(1 * time.Second)
+	mClock.Advance(1 * time.Second).MustWait(ctx)
+	tkr.Stop()
+
+	select {
+	case tme := <-tkr.C:
+		t.Fatalf("got channel read after stop: %s", tme)
+	default:
+		// OK!
+	}
+}

ticker.go

@@ -6,12 +6,25 @@ import "time"
 type Ticker struct {
 	C <-chan time.Time
 	//nolint: revive
-	c       chan time.Time
-	ticker  *time.Ticker  // realtime impl, if set
-	d       time.Duration // period, if set
-	nxt     time.Time     // next tick time
-	mock    *Mock         // mock clock, if set
-	stopped bool          // true if the ticker is not running
+	c             chan time.Time
+	ticker        *time.Ticker   // realtime impl, if set
+	d             time.Duration  // period, if set
+	nxt           time.Time      // next tick time
+	mock          *Mock          // mock clock, if set
+	stopped       bool           // true if the ticker is not running
+	internalTicks chan time.Time // used to deliver ticks to the runLoop goroutine
+
+	// As of Go 1.23, ticker channels are unbuffered and guaranteed to block forever after a call to stop.
+	//
+	// When a mocked ticker 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 (runLoop) if we are a
+	// channel-based ticker.
+	//
+	// 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 runLoop goroutine.
+	interrupt chan struct{}
 }
 
 func (t *Ticker) fire(tt time.Time) {
@@ -24,9 +37,8 @@ func (t *Ticker) fire(tt time.Time) {
 		t.nxt = t.nxt.Add(t.d)
 	}
 	t.mock.recomputeNextLocked()
-	select {
-	case t.c <- tt:
-	default:
+	if t.interrupt != nil { // implies runLoop is still going.
+		t.internalTicks <- tt
 	}
 }
 
@@ -49,6 +61,11 @@ func (t *Ticker) Stop(tags ...string) {
 	defer close(c.complete)
 	t.mock.removeEventLocked(t)
 	t.stopped = true
+	// check if we've already fired, and if so, interrupt it.
+	if t.interrupt != nil {
+		<-t.interrupt
+		t.interrupt = nil
+	}
 }
 
 // Reset stops a ticker and resets its period to the specified duration. The
@@ -72,4 +89,63 @@ func (t *Ticker) Reset(d time.Duration, tags ...string) {
 	} else {
 		t.mock.recomputeNextLocked()
 	}
+	if t.interrupt == nil {
+		t.startRunLoopLocked()
+	}
+}
+
+func (t *Ticker) runLoop(interrupt chan struct{}) {
+	defer close(interrupt)
+outer:
+	for {
+		select {
+		case tt := <-t.internalTicks:
+			for {
+				select {
+				case t.c <- tt:
+					continue outer
+				case <-t.internalTicks:
+					// Discard future ticks until we can send this one.
+				case interrupt <- struct{}{}:
+					return
+				}
+			}
+		case interrupt <- struct{}{}:
+			return
+		}
+	}
+}
+
+func (t *Ticker) startRunLoopLocked() {
+	// assert some assumptions. If these fire, it is a bug in Quartz itself.
+	if t.interrupt != nil {
+		t.mock.tb.Error("called startRunLoopLocked when interrupt suggests we are already running")
+	}
+	interrupt := make(chan struct{})
+	t.interrupt = interrupt
+	go t.runLoop(interrupt)
+}
+
+func newMockTickerLocked(m *Mock, d time.Duration) *Ticker {
+	// no buffer follows Go 1.23+ behavior
+	ticks := make(chan time.Time)
+	t := &Ticker{
+		C:             ticks,
+		c:             ticks,
+		d:             d,
+		nxt:           m.cur.Add(d),
+		mock:          m,
+		internalTicks: make(chan time.Time),
+	}
+	m.addEventLocked(t)
+	m.tb.Cleanup(func() {
+		m.mu.Lock()
+		defer m.mu.Unlock()
+		if t.interrupt != nil {
+			<-t.interrupt
+			t.interrupt = nil
+		}
+	})
+	t.startRunLoopLocked()
+	return t
 }

timer.go

@@ -1,6 +1,8 @@
 package quartz
 
-import "time"
+import (
+	"time"
+)
 
 // The Timer type represents a single event. When the Timer expires, the current time will be sent
 // on C, unless the Timer was created by AfterFunc. A Timer must be created with NewTimer or
@@ -14,14 +16,43 @@ type Timer struct {
 	mock    *Mock       // mock clock, if set
 	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.
+	//
+	// 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.
+	interrupt chan struct{}
 }
 
 func (t *Timer) fire(tt time.Time) {
-	t.mock.removeTimer(t)
+	t.mock.mu.Lock()
+	t.mock.removeTimerLocked(t)
 	if t.fn != nil {
+		t.mock.mu.Unlock()
 		t.fn()
+		return
 	} else {
-		t.c <- tt
+		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.mu.Unlock()
+		go func() {
+			defer close(interrupt)
+			select {
+			case t.c <- tt:
+			case interrupt <- struct{}{}:
+			}
+		}()
 	}
 }
 
@@ -45,6 +76,11 @@ 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
+	}
 	return result
 }
 
@@ -62,9 +98,10 @@ func (t *Timer) Reset(d time.Duration, tags ...string) bool {
 	t.mock.matchCallLocked(c)
 	defer close(c.complete)
 	result := !t.stopped
-	select {
-	case <-t.c:
-	default:
+	// check if we've already fired, and if so, interrupt it.
+	if t.interrupt != nil {
+		<-t.interrupt
+		t.interrupt = nil
 	}
 	if d <= 0 {
 		// zero or negative duration timer means we should immediately re-fire