time: make Timer/Ticker channels not receivable with old values after Stop or Reset returns

[bcmills]

In #14383 (comment), @rsc said:

There is an argument for making

tr := time.NewTimer(0)
tr.Stop()
// There may or may not be a value in the channel now. But definitely
// one should not be added after we receive it.
select {
        case <-tr.C:
        default:
}

work, but it doesn't today, and we're certainly not going to do that for Go 1.7. Maybe early in Go 1.8 if someone wants to file a different bug.

As far as I can tell, no different bug was filed: the documentation for (*Timer).Stop still says:

go/src/time/sleep.go

Lines 57 to 66 in 7d2473d

	// 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.

and that behavior is still resulting in subtle bugs (#27169). (Note that @rsc himself assumed that a select should work in this way in #14038 (comment).)

I think we should tighten up the invariants of (*Timer).Stop to eliminate this subtlety.

CC @empijei @matttproud @the80srobot @ianlancetaylor

[bcmills]

For an example program illustrating why a user might reasonably expect (and attempt to make use of) this behavior, see https://play.golang.org/p/mDkMG67ehAI.

[empijei]

Thanks for filing this.

I also found this behavior puzzling and apparently I got it wrong too as I was expecting no values to be sent after t.Stop() returned.

If, as I understand now, a value might be sent after the call to Stop has returned, is there any way to correctly re-use a timer?

[bcmills]

If, as I understand now, a value might be sent after the call to Stop has returned, is there any way to correctly re-use a timer?

If you know that no other goroutine is receiving on the C channel, then you can receive unconditionally after the call to Stop returns false, and it won't actually matter whether the value was delivered before or after the call to Stop returned.

(The select-with-default after Stop is where the bug occurs, and none of the examples in your post do that — they don't need to, because in your examples the reads on t.C are all on the same goroutine as the calls to t.Stop.)

[ianlancetaylor]

In the current implementation, this can probably be done by changing runOneTimer to move the Cas away from timerRunning until after the invocation of f.

[rsc]

Added to proposal process to resolve whether to do this, after mention in #38945.

[rsc]

This made its first appearance in the proposal minutes last week.
You can imagine ways to break Go programs with this, such as by

t := time.NewTimer(1*time.Second)
time.Sleep(2*time.Second)
t.Stop()
<-t.C

This would succeed today, but if we clear the channel during Stop it would start blocking.
We would have to define that in the case where Stop pulls the element back out of the channel buffer, it returns true, meaning it "stopped" the timer (false means the timer already expired; expired would now mean "expired and was observed by a channel receive on t.C").

On the other hand, there is technically a race here, and it's not guaranteed that this snippet never blocks, especially on a loaded system. The correct snippet, even today, is:

t := time.NewTimer(1*time.Second)
time.Sleep(2*time.Second)
if !t.Stop() {
    <-t.C
}

That example would be unaffected, because Stop would pull the buffered element back out of the channel and then return true, signaling to the caller that there is nothing to receive.

We could make the change at the start of a release cycle and be ready to roll it back if real (as opposed to hypothetical) problems arose.

Thoughts?