time: avoid stale receives after Timer/Ticker Stop/Reset return

A proposal discussion in mid-2020 on #37196 decided to change
time.Timer and time.Ticker so that their Stop and Reset methods
guarantee that no old value (corresponding to the previous configuration
of the Timer or Ticker) will be received after the method returns.

The trivial way to do this is to make the Timer/Ticker channels
unbuffered, create a goroutine per Timer/Ticker feeding the channel,
and then coordinate with that goroutine during Stop/Reset.
Since Stop/Reset coordinate with the goroutine and the channel
is unbuffered, there is no possibility of a stale value being sent
after Stop/Reset returns.

Of course, we do not want an extra goroutine per Timer/Ticker,
but that's still a good semantic model: behave like the channels
are unbuffered and fed by a coordinating goroutine.

The actual implementation is more effort but behaves like the model.
Specifically, the timer channel has a 1-element buffer like it always has,
but len(t.C) and cap(t.C) are special-cased to return 0 anyway, so user
code cannot see what's in the buffer except with a receive.
Stop/Reset lock out any stale sends and then clear any pending send
from the buffer.

Some programs will change behavior. For example:

	package main

	import "time"

	func main() {
		t := time.NewTimer(2 * time.Second)
		time.Sleep(3 * time.Second)
		if t.Reset(2*time.Second) != false {
			panic("expected timer to have fired")
		}
		<-t.C
		<-t.C
	}

This program (from #11513) sleeps 3s after setting a 2s timer,
resets the timer, and expects Reset to return false: the Reset is too
late and the send has already occurred. It then expects to receive
two values: the one from before the Reset, and the one from after
the Reset.

With an unbuffered timer channel, it should be clear that no value
can be sent during the time.Sleep, so the time.Reset returns true,
indicating that the Reset stopped the timer from going off.
Then there is only one value to receive from t.C: the one from after the Reset.

In 2015, I used the above example as an argument against this change.

Note that a correct version of the program would be:

	func main() {
		t := time.NewTimer(2 * time.Second)
		time.Sleep(3 * time.Second)
		if !t.Reset(2*time.Second) {
			<-t.C
		}
		<-t.C
	}

This works with either semantics, by heeding t.Reset's result.
The change should not affect correct programs.

However, one way that the change would be visible is when programs
use len(t.C) (instead of a non-blocking receive) to poll whether the timer
has triggered already. We might legitimately worry about breaking such
programs.

In 2020, discussing #37196, Bryan Mills and I surveyed programs using
len on timer channels. These are exceedingly rare to start with; nearly all
the uses are buggy; and all the buggy programs would be fixed by the new
semantics. The details are at [1].

To further reduce the impact of this change, this CL adds a temporary
GODEBUG setting, which we didn't know about yet in 2015 and 2020.
Specifically, asynctimerchan=1 disables the change and is the default
for main programs in modules that use a Go version before 1.23.
We hope to be able to retire this setting after the minimum 2-year window.
Setting asynctimerchan=1 also disables the garbage collection change
from CL 568341, although users shouldn't need to know that since
it is not a semantically visible change (unless we have bugs!).

As an undocumented bonus that we do not officially support,
asynctimerchan=2 disables the channel buffer change but keeps
the garbage collection change. This may help while we are
shaking out bugs in either of them.

Fixes #37196.

[1] https://github.com/golang/go/issues/37196#issuecomment-641698749

Change-Id: I8925d3fb2b86b2ae87fd2acd055011cbf7bd5916
Reviewed-on: https://go-review.googlesource.com/c/go/+/568341
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

2 files changed, 174 insertions, 101 deletions

diff --git a/src/time/sleep.go b/src/time/sleep.go
index e6225dfb35..669660f90e 100644
--- a/src/time/sleep.go
+++ b/src/time/sleep.go
@@ -27,6 +27,20 @@ func syncTimer(c chan Time) unsafe.Pointer {
 	}
 
 	// Otherwise pass to runtime.
+	// This handles asynctimerchan=0, which is the default Go 1.23 behavior,
+	// as well as asynctimerchan=2, which is like asynctimerchan=1
+	// but implemented entirely by the runtime.
+	// The only reason to use asynctimerchan=2 is for debugging
+	// a problem fixed by asynctimerchan=1: it enables the new
+	// GC-able timer channels (#61542) but not the sync channels (#37196).
+	//
+	// If we decide to roll back the sync channels, we will still have
+	// a fully tested async runtime implementation (asynctimerchan=2)
+	// and can make this function always return c.
+	//
+	// If we decide to keep the sync channels, we can delete all the
+	// handling of asynctimerchan in the runtime and keep just this
+	// function to handle asynctimerchan=1.
 	return *(*unsafe.Pointer)(unsafe.Pointer(&c))
 }
 
@@ -79,25 +93,22 @@ type Timer struct {
 // 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.
 //
-// To ensure the channel is empty after a call to Stop, check the
-// return value and drain the channel.
-// For example, assuming the program has not received from t.C already:
-//
-//	if !t.Stop() {
-//		<-t.C
-//	}
-//
-// This cannot be done concurrent to other receives from the Timer's
-// channel or other calls to the Timer's Stop method.
-//
-// For a timer created with AfterFunc(d, f), if t.Stop returns false, then the timer
-// has already expired and the function f has been started in its own goroutine;
+// For a func-based timer created with AfterFunc(d, f),
+// if t.Stop returns false, then the timer has already expired
+// and the function f has been started in its own goroutine;
 // Stop does not wait for f to complete before returning.
-// If the caller needs to know whether f is completed, it must coordinate
-// with f explicitly.
+// If the caller needs to know whether f is completed,
+// it must coordinate with f explicitly.
+//
+// For a chan-based timer created with NewTimer(d), as of Go 1.23,
+// any receive from t.C after Stop has returned is guaranteed to block
+// rather than receive a stale time value from before the Stop;
+// if the program has not received from t.C already and the timer is
+// running, Stop is guaranteed to return true.
+// Before Go 1.23, the only safe way to use Stop was insert an extra
+// <-t.C if Stop returned false to drain a potential stale value.
+// See the [NewTimer] documentation for more details.
 func (t *Timer) Stop() bool {
 	if !t.initTimer {
 		panic("time: Stop called on uninitialized Timer")
@@ -116,6 +127,18 @@ func (t *Timer) Stop() bool {
 // timers, even if they haven't expired or been stopped.
 // The Stop method is no longer necessary to help the garbage collector.
 // (Code may of course still want to call Stop to stop the timer for other reasons.)
+//
+// Before Go 1.23, the channel assocated with a Timer was
+// asynchronous (buffered, capacity 1), which meant that
+// stale time values could be received even after [Timer.Stop]
+// or [Timer.Reset] returned.
+// As of Go 1.23, the channel is synchronous (unbuffered, capacity 0),
+// eliminating the possibility of those stale values.
+//
+// The GODEBUG setting asynctimerchan=1 restores both pre-Go 1.23
+// behaviors: when set, unexpired timers won't be garbage collected, and
+// channels will have buffered capacity. This setting may be removed
+// in Go 1.27 or later.
 func NewTimer(d Duration) *Timer {
 	c := make(chan Time, 1)
 	t := (*Timer)(newTimer(when(d), 0, sendTime, c, syncTimer(c)))
@@ -127,29 +150,7 @@ func NewTimer(d Duration) *Timer {
 // It returns true if the timer had been active, false if the timer had
 // expired or been stopped.
 //
-// For a Timer created with NewTimer, Reset should be invoked only on
-// stopped or expired timers with drained channels.
-//
-// If a program has already received a value from t.C, the timer is known
-// to have expired and the channel drained, so t.Reset can be used directly.
-// If a program has not yet received a value from t.C, however,
-// the timer must be stopped and—if Stop reports that the timer expired
-// before being stopped—the channel explicitly drained:
-//
-//	if !t.Stop() {
-//		<-t.C
-//	}
-//	t.Reset(d)
-//
-// This should not be done concurrent to other receives from the Timer's
-// channel.
-//
-// Note that it is not possible to use Reset's return value correctly, as there
-// is a race condition between draining the channel and the new timer expiring.
-// Reset should always be invoked on stopped or expired channels, as described above.
-// The return value exists to preserve compatibility with existing programs.
-//
-// For a Timer created with AfterFunc(d, f), Reset either reschedules
+// For a func-based timer created with AfterFunc(d, f), Reset either reschedules
 // when f will run, in which case Reset returns true, or schedules f
 // to run again, in which case it returns false.
 // When Reset returns false, Reset neither waits for the prior f to
@@ -157,6 +158,15 @@ func NewTimer(d Duration) *Timer {
 // goroutine running f does not run concurrently with the prior
 // one. If the caller needs to know whether the prior execution of
 // f is completed, it must coordinate with f explicitly.
+//
+// For a chan-based timer created with NewTimer, as of Go 1.23,
+// any receive from t.C after Reset has returned is guaranteed not
+// to receive a time value corresponding to the previous timer settings;
+// if the program has not received from t.C already and the timer is
+// running, Reset is guaranteed to return true.
+// Before Go 1.23, the only safe way to use Reset was to Stop and
+// explicitly drain the timer first.
+// See the [NewTimer] documentation for more details.
 func (t *Timer) Reset(d Duration) bool {
 	if !t.initTimer {
 		panic("time: Reset called on uninitialized Timer")
diff --git a/src/time/tick_test.go b/src/time/tick_test.go
index 46268acfe3..90c13fbe82 100644
--- a/src/time/tick_test.go
+++ b/src/time/tick_test.go
@@ -306,38 +306,52 @@ func TestTimerGC(t *testing.T) {
 	run(t, "NewTickerStop", func() { NewTicker(Hour).Stop() })
 }
 
-func TestTimerChan(t *testing.T) {
-	t.Parallel()
-	tick := &timer2{NewTimer(10000 * Second)}
-	testTimerChan(t, tick, tick.C)
+func TestChan(t *testing.T) {
+	for _, name := range []string{"0", "1", "2"} {
+		t.Run("asynctimerchan="+name, func(t *testing.T) {
+			t.Setenv("GODEBUG", "asynctimerchan="+name)
+			t.Run("Timer", func(t *testing.T) {
+				tim := NewTimer(10000 * Second)
+				testTimerChan(t, tim, tim.C, name == "0")
+			})
+			t.Run("Ticker", func(t *testing.T) {
+				tim := &tickerTimer{Ticker: NewTicker(10000 * Second)}
+				testTimerChan(t, tim, tim.C, name == "0")
+			})
+		})
+	}
 }
 
-func TestTickerChan(t *testing.T) {
-	t.Parallel()
-	tick := NewTicker(10000 * Second)
-	testTimerChan(t, tick, tick.C)
+type timer interface {
+	Stop() bool
+	Reset(Duration) bool
 }
 
-// timer2 is a Timer with Reset and Stop methods with no result,
-// to have the same signatures as Ticker.
-type timer2 struct {
-	*Timer
+// tickerTimer is a Timer with Reset and Stop methods that return bools,
+// to have the same signatures as Timer.
+type tickerTimer struct {
+	*Ticker
+	stopped bool
 }
 
-func (t *timer2) Stop() {
-	t.Timer.Stop()
+func (t *tickerTimer) Stop() bool {
+	pending := !t.stopped
+	t.stopped = true
+	t.Ticker.Stop()
+	return pending
 }
 
-func (t *timer2) Reset(d Duration) {
-	t.Timer.Reset(d)
+func (t *tickerTimer) Reset(d Duration) bool {
+	pending := !t.stopped
+	t.stopped = false
+	t.Ticker.Reset(d)
+	return pending
 }
 
-type ticker interface {
-	Stop()
-	Reset(Duration)
-}
+func testTimerChan(t *testing.T, tim timer, C <-chan Time, synctimerchan bool) {
+	_, isTimer := tim.(*Timer)
+	isTicker := !isTimer
 
-func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 	// Retry parameters. Enough to deflake even on slow machines.
 	// Windows in particular has very coarse timers so we have to
 	// wait 10ms just to make a timer go off.
@@ -357,7 +371,7 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 		select {
 		default:
 		case <-C:
-			t.Fatalf("extra tick")
+			t.Errorf("extra tick")
 		}
 	}
 	assertTick := func() {
@@ -375,10 +389,16 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 				return
 			}
 		}
-		t.Fatalf("missing tick")
+		t.Errorf("missing tick")
 	}
 	assertLen := func() {
 		t.Helper()
+		if synctimerchan {
+			if n := len(C); n != 0 {
+				t.Errorf("synctimer has len(C) = %d, want 0 (always)", n)
+			}
+			return
+		}
 		var n int
 		if n = len(C); n == 1 {
 			return
@@ -389,50 +409,58 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 				return
 			}
 		}
-		t.Fatalf("len(C)  = %d, want 1", n)
+		t.Errorf("len(C) = %d, want 1", n)
 	}
 
 	// Test simple stop; timer never in heap.
-	tick.Stop()
+	tim.Stop()
 	noTick()
 
 	// Test modify of timer not in heap.
-	tick.Reset(10000 * Second)
+	tim.Reset(10000 * Second)
 	noTick()
 
-	// Test modify of timer in heap.
-	tick.Reset(1)
-	assertTick()
-
-	// Sleep long enough that a second tick must happen if this is a ticker.
-	// Test that Reset does not lose the tick that should have happened.
-	Sleep(sched)
-	tick.Reset(10000 * Second)
-	_, isTicker := tick.(*Ticker)
-	if isTicker {
+	if synctimerchan {
+		// Test modify of timer in heap.
+		tim.Reset(1)
+		Sleep(sched)
+		if l, c := len(C), cap(C); l != 0 || c != 0 {
+			//t.Fatalf("len(C), cap(C) = %d, %d, want 0, 0", l, c)
+		}
 		assertTick()
-	}
-	noTick()
+	} else {
+		// Test modify of timer in heap.
+		tim.Reset(1)
+		assertTick()
+		Sleep(sched)
+		tim.Reset(10000 * Second)
+		if isTicker {
+			assertTick()
+		}
+		noTick()
 
-	// Test that len sees an immediate tick arrive
-	// for Reset of timer in heap.
-	tick.Reset(1)
-	assertLen()
-	assertTick()
+		// Test that len sees an immediate tick arrive
+		// for Reset of timer in heap.
+		tim.Reset(1)
+		assertLen()
+		assertTick()
 
-	// Test that len sees an immediate tick arrive
-	// for Reset of timer NOT in heap.
-	tick.Stop()
-	drain()
-	tick.Reset(1)
-	assertLen()
-	assertTick()
+		// Test that len sees an immediate tick arrive
+		// for Reset of timer NOT in heap.
+		tim.Stop()
+		if !synctimerchan {
+			drain()
+		}
+		tim.Reset(1)
+		assertLen()
+		assertTick()
+	}
 
 	// Sleep long enough that a second tick must happen if this is a ticker.
 	// Test that Reset does not lose the tick that should have happened.
 	Sleep(sched)
-	tick.Reset(10000 * Second)
-	if isTicker {
+	tim.Reset(10000 * Second)
+	if !synctimerchan && isTicker {
 		assertLen()
 		assertTick()
 	}
@@ -461,8 +489,10 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 	}
 
 	// Reset timer in heap (already reset above, but just in case).
-	tick.Reset(10000 * Second)
-	drain()
+	tim.Reset(10000 * Second)
+	if !synctimerchan {
+		drain()
+	}
 
 	// Test stop while timer in heap (because goroutine is blocked on <-C).
 	done := make(chan bool)
@@ -475,12 +505,12 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 	notDone(done)
 
 	// Test reset far away while timer in heap.
-	tick.Reset(20000 * Second)
+	tim.Reset(20000 * Second)
 	Sleep(sched)
 	notDone(done)
 
 	// Test imminent reset while in heap.
-	tick.Reset(1)
+	tim.Reset(1)
 	waitDone(done)
 
 	// If this is a ticker, another tick should have come in already
@@ -491,14 +521,18 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 		noTick()
 	}
 
-	tick.Stop()
-	if isTicker {
+	tim.Stop()
+	if isTicker || !synctimerchan {
+		t.Logf("drain")
 		drain()
 	}
 	noTick()
 
 	// Again using select and with two goroutines waiting.
-	tick.Reset(10000 * Second)
+	tim.Reset(10000 * Second)
+	if !synctimerchan {
+		drain()
+	}
 	done = make(chan bool, 2)
 	done1 := make(chan bool)
 	done2 := make(chan bool)
@@ -521,10 +555,10 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 	}()
 	Sleep(sched)
 	notDone(done)
-	tick.Reset(sched / 2)
+	tim.Reset(sched / 2)
 	Sleep(sched)
 	waitDone(done)
-	tick.Stop()
+	tim.Stop()
 	close(stop)
 	waitDone(done1)
 	waitDone(done2)
@@ -560,6 +594,35 @@ func testTimerChan(t *testing.T, tick ticker, C <-chan Time) {
 	close(stop)
 	waitDone(done)
 	waitDone(done)
+
+	// Test that Stop and Reset block old values from being received.
+	// (Proposal go.dev/issue/37196.)
+	if synctimerchan {
+		tim.Reset(1)
+		Sleep(10 * Millisecond)
+		if pending := tim.Stop(); pending != true {
+			t.Errorf("tim.Stop() = %v, want true", pending)
+		}
+		noTick()
+
+		tim.Reset(Hour)
+		noTick()
+		if pending := tim.Reset(1); pending != true {
+			t.Errorf("tim.Stop() = %v, want true", pending)
+		}
+		assertTick()
+		Sleep(10 * Millisecond)
+		if isTicker {
+			assertTick()
+			Sleep(10 * Millisecond)
+		} else {
+			noTick()
+		}
+		if pending, want := tim.Reset(Hour), isTicker; pending != want {
+			t.Errorf("tim.Stop() = %v, want %v", pending, want)
+		}
+		noTick()
+	}
 }
 
 func TestManualTicker(t *testing.T) {