-
Notifications
You must be signed in to change notification settings - Fork 17.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime: timer.Stop returns false even when no value is read from the channel #69312
Comments
From
So even if you don't read from the channel, but the timer is not running -> Stop could return false. |
The text of the issue talks about |
It's with both: Both the examples |
I've simplified the examples: Stop: https://go.dev/play/p/ofa64fUdVBT |
Thanks. In https://go.dev/play/p/ofa64fUdVBT there is a race between running the timer and stopping the timer. The code requires that If I understand you correctly, you are saying that because the timer is running, and because nothing has received from the channel, then The code you point to in runtime/time.go is intended to handle exactly the case: a timer that has expired racing with a call to So it's not yet obvious to me that there is a bug here. We have to handle the race somehow. We want to be sure that after Or so it seems to me. What am I missing? Thanks. |
Thanks @ianlancetaylor, @cuonglm. Some thoughts.
I believe this is a bug because in the changelist entry that introduced this change there is this example:
This example also suffers from the same race condition. The same argument for
The timer only "expires" when the value is read from the channel, no?
This applies to @cuonglm 's point too
I'm uncomfortable with this definition of not running
If Why is the timer not running when
|
OK, I think I see what you are getting at. In the racing case, |
@gopherbot Please open a backport to 1.23. This bug makes it difficult or impossible to write timer code that uses Stop and Reset and works correctly for all versions of Go. |
Backport issue(s) opened: #69333 (for 1.23). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
Change https://go.dev/cl/611496 mentions this issue: |
Until golang/go#69312 is resolved, force the old timer behavior by specifying an older go version in the go.mod file.
Until golang/go#69312 is resolved, force the old timer behavior by specifying an older go version in the go.mod file. Fixes #4606
Go version
go1.23.0 linux/amd64
Output of
go env
in your module/workspace:What did you do?
For the new unbuffered timers introduced in go1.23, I expected that
timer.Stop
would return true, if no value is read from timer channel. This is not the case, sometimestimer.Stop
returns false even when no value is read from the channel and the timer wasn't stopped before. https://go.dev/play/p/t_vaPhSAkmwIn production this is breaking quic-go: quic-go/quic-go#4659
There are details in that PR about how it's breaking Kubo v0.30.0-rc2(https://github.com/ipfs/kubo/tree/v0.30.0-rc2)
What did you see happen?
timer.Stop
returns false even if no value was read. And there's no corresponding value in the timer channel. This breaks existing code which relies on tracking whether it read from the timer channel or not like the one in quic-go here: https://github.com/quic-go/quic-go/blob/master/internal/utils/timer.goWhat did you expect to see?
I expected it to always return true if no value was read from the channel.
Looking at the changelog: https://go-review.googlesource.com/c/go/+/568341 there's an example of correct timer usage that shouldn't be broken.
I believe this will break(block forever) too in some circumstances. From the code we can see why this would happen.
unlockAndRun
updates the timer state and releases timer lock.modify
readst.when == 0
and returns false, thenunlockAndRun
gets thesendLock
but cancels the push to the channel.Repro here: https://go.dev/play/p/y_PZbPlwqrM
The text was updated successfully, but these errors were encountered: