go: time: make Timer/Ticker channels not receivable with old values after Stop or Reset returns
In https://github.com/golang/go/issues/14383#issuecomment-220657111, @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:
https://github.com/golang/go/blob/7d2473dc81c659fba3f3b83bc6e93ca5fe37a898/src/time/sleep.go#L57-L66
and that behavior is still resulting in subtle bugs (#27169). (Note that @rsc himself assumed that a select
should work in this way in https://github.com/golang/go/issues/14038#issuecomment-219909704.)
I think we should tighten up the invariants of (*Timer).Stop
to eliminate this subtlety.
About this issue
- Original URL
- State: open
- Created 4 years ago
- Reactions: 11
- Comments: 25 (25 by maintainers)
For what it’s worth, seeing all that buggy code as evidence of people struggling to use the current t.Stop and t.Reset correctly in single-goroutine use cases only makes me that much more convinced we should try the proposed change.
Above, we identified that one possible way that the semantic change being discussed might break code is if the code used
len
to poll whether the timer had gone off. We would have to makelen
report zero all the time, simulating an unbuffered channel. Code that depended onlen
sometimes being non-zero could possibly break.My intuition was that using
len
on a channel is racy and almost never better than a select with a default case, so there’s a decent chance that not too much code is using len. To test this hypothesis, I searched my local corpus of Go packages for length on timer channels and sifted through the results with @bcmills. We found that use oflen
on timer channels is (1) rare, (2) almost always incorrect, (3) when incorrect, always fixed by the new semantics.The corpus I am using is a collection of the latest of each major version of each module listed in the Go index (index.golang.org) as of late March. There are 134,485 modules with distinct paths in my corpus. Of those, 17,021 mention
time.NewTimer
ortime.NewTicker
. Of those, 15 modules (slightly under 0.1% of the 17,021) contain code matchinglen\(.*\.C\)
in which thelen
operation applies to a timer or ticker channel.Of the 15 modules applying
len
to a timer or ticker channel: one is an awkward simulation of a non-blocking select, already removed in a later commit; one useslen
in debug prints to monitor the ticker; and the remaining 13 are buggy.The semantic change proposed in this issue would make all the buggy code correct, assuming we apply the same change to
Reset
as we’ve discussed forStop
. We should probably also apply the change totime.Ticker
’sReset
andStop
.Use of len to simulate non-blocking select
github.com/packetzoom/logslammer
github.com/packetzoom/logslammer version v0.0.3 output/elasticsearch/elasticsearch.go (April 7, 2016) contains the following code in output/elasticsearch/elasticsearch.go:
This code seems to be using the
len
checks as an alternative to a non-blockingselect
on those two channels. It’s also worth noting that when received events are rare, the tick receive and correspondingidx.flush
may be arbitrarily delayed by the blocking receive inreadInputChannel
. It’s possible that select was meant to be non-blocking.The semantic change proposed in this issue would make the code never process ticks, since
len(tick.C)
would always be zero. However, in the latest untagged commit (February 17, 2017), the test oflen(tick.C)
has been removed. Also, the lack of any commits in the last three years, nor even a tag of the updated code, suggests the code is unmaintained and no longer used.Use of len for logging/debugging prints
github.com/uber/cherami-server
github.com/uber/cherami-server version v1.28.1 services/outputhost/messagecache.go (November 29, 2017) has a
logPumpHealth
method that logs information about the capacity and length of its ticker channel, among others:And the `updatePumpHealth method does:
The logging would never trigger anymore, and the pump health update would register the channel as “full” for being unbuffered (length and capacity both 0).
However, the repository’s README says that the project is “deprecated and not maintained.”
Racy use of len to drain channel after Stop
github.com/Freezerburn/go-coroutine
github.com/Freezerburn/go-coroutine v1.0.1 src/github.com/Freezerburn/coroutine/embed.go:
This use of
len
is potentially racing with the pending send thatStop
did not stop. The correct code today would be to track whethere.waitTimer.C
has been received from already and use that state in place of the length check. In fact, since this code is the only use ofe.waitTimer
, the timer will have only already fired and not been received from on the first execution of the method containing this code. Every time the method completes, the timer has expired and been read from. The first execution is different becausee.waitTimer
is initialized totime.NewTimer(0)
.The proposed semantic change to
Stop
would make this code correct: on the first call,e.waitTimer.Stop
would return true—theStop
prevented the zero-duration timer event from being sent. On subsequent iterations,e.waitTimer.Stop
would return false—the timer event was already received by the previous call—andlen
would be zero, bypassing the no-longer-necessary draining code.There is another instance of the pattern later in the file:
Like
e.waitTimer
,e.receiveTimer
is initialized totime.NewTimer(0)
. On top of that, the code does not record whether the select received frome.receiveTimer.C
, as it must for proper interpretation of theStop
result. This lack of memory is partly compensated for by the racy (buggy)len
check.The same analysis applies: if the
Stop
guarantees to stop any future send, then either the receive has already happened, in which caseStop
returns false andlen
is zero, so or the receive is prevented, in which caseStop
returns true. Either way, the code would now bypass the no-longer-necessary draining code.The current latest commit rewrites both of those code snippets to use a helper that is different (avoiding
len
entirely) but still buggy:The same analysis applies, since a non-blocking select is just a less racy “
len
plus receive” operation. Removing the race between the length check and the receive did not eliminate the race with the delayed timer send. But the proposed semantic change would.github.com/MatrixAINetwork/go-matrix
github.com/MatrixAINetwork/go-matrix v1.1.7 p2p/buckets.go (May 20, 2020):
This code is trying to make sure that each iteration of the select falls into the timeout case after 60 seconds. For the most part it succeeds, but a receive from
b.blockChain
right at the moment the timer went off would fail to draintimeoutTimer.C
, causing the next iteration to fall into the timeout case immediately.Same race as in previous example; proposed semantic change applies the same fix.
github.com/cobolbaby/log-agent
github.com/cobolbaby/log-agent 71f7f9f watchdog/watchdog.go:
Same race; same fix.
github.com/dcarbone/go-cacheman
github.com/dcarbone/go-cacheman v1.1.2 key_manager.go:
Same race; same fix (appears four times in file).
github.com/myENA/consultant/v2
github.com/myENA/consultant/v2 v2.1.1 service.go:
Same race; same fix (appears three times in file).
github.com/smartcontractkit/chainlink
github.com/smartcontractkit/chainlink v0.7.8 core/services/fluxmonitor/flux_monitor.go:
Same race; same fix. This is of course not the code suggested by the documentation, although it does match the (incorrect) linked blog post.
This code was removed without comment in v0.8.0.
github.com/vntchain/go-vnt
github.com/vntchain/go-vnt v0.6.4-alpha.6 producer/worker.go:
Same race; same fix.
github.com/eBay/akutan
github.com/eBay/akutan 9a750f2 src/github.com/ebay/akutan/util/clocks/wall.go:
Same race; same fix.
github.com/piotrnar/gocoin
github.com/piotrnar/gocoin 820d7ad client/main.go:
Same race; same fix.
Racy use of len to drain channel before Reset
github.com/qlcchain/go-qlc
github.com/qlcchain/go-qlc v1.3.5 p2p/sync.go:
This is the usual pattern from the previous section, except there is no call to
Stop
, so the receives are racing against an unstopped timer instead of the dying gasps of a stopped timer. But the race is the same.If we change
Reset
to have the same guarantee asStop
, namely that no send from the old timer setting (before theReset
) can complete afterReset
returns, then a plain call toReset
withoutStop
like in this code becomes correct, even without theif
statement. With the proposed change, theif
body would never execute.Racy use of len to drain channel after Reset
github.com/chenjie199234/Corelib
github.com/chenjie199234/Corelib 2d8c16542cbe logger/logger.go:
This is like the previous example but the draining happens after
Reset
, making it race with the newly reset timer. A bad scheduling pause that introduced a half-second delay in goroutine execution between the timer being reset and the draining would cause a legitimate timer event to be discarded.This entire package was removed on April 20.
Racy use of len to drain channel without Stop/Reset
github.com/indeedsecurity/carbonbeat
github.com/indeedsecurity/carbonbeat v1.0.0 app/carbonbeat.go:
Strictly speaking, this code is not wrong, but it is also not useful. The ticker channel has a buffer of length 1. In the instant after the receive just completed, the buffer is almost certainly empty. The check is racing with the next send, but very unlikely to lose that race. Changing the
len
to always return 0 will not have any discernible impact on this code.github.com/indeedsecurity/carbonbeat/v2
github.com/indeedsecurity/carbonbeat/v2 v2.0.2 app/carbonbeat.go contains the same code.
Discussion ongoing, so leaving this for another week, but it seems headed for likely accept. Please speak up if you think this is a bad idea.
In the current implementation, this can probably be done by changing
runOneTimer
to move theCas
away fromtimerRunning
until after the invocation off
.I think this is an excellent idea and I’d bet that I’ve written at least one piece of code which lacks, but needs, the fixup code.
Added to proposal process to resolve whether to do this, after mention in #38945.