The following code works fine on Go 1.22, but breaks on Go 1.23.0 on the http.DefaultClient.Do() line. On Linux, it freezes the entire runtime and never continues. On macOS, it throws a fatal error: ts set in timer
package main
import (
"fmt"
"net/http"
"time"
)
func main() {
illegalTimerCopy := *time.NewTimer(10 * time.Second)
go func() {
illegalTimerCopy.Reset(10 * time.Second)
}()
req, err := http.NewRequest(http.MethodGet, "https://example.com", nil)
fmt.Println("Request created", err)
_, err = http.DefaultClient.Do(req)
fmt.Println("Request completed", err)
}
It's presumably somehow caused by the Go 1.23 timer changes, but I'm not sure how exactly, so I don't know if it's a bug or a feature. Assuming it's not a bug, it would be nice if go vet could report that similar to the mutex copy warnings.
Comment From: zigo101
package main
import "time"
func main() {
illegalTimerCopy := *time.NewTimer(time.Second)
illegalTimerCopy.Stop() // block for ever
}
Comment From: ianlancetaylor
I'm kind of surprised this ever worked. I don't think it would work in general cases. It might work in cases where you are careful to call Reset after the copy before doing anything else.
Comment From: ianlancetaylor
Unless we get a lot of reports about breakage due to this, we aren't going to change it. So changing this to a proposal for a vet check. Though I don't know how useful a vet check would be--I don't know how often this occurs. At least since Go 1.4 the time.Timer docs have said
// A Timer must be created with NewTimer or AfterFunc.
That doc was added for #8776. So I think this has always been documented as not working.
Comment From: timothy-king
If we do decide to add it to vet, copylock is a natural home for implementing this (if a mildly inappropriate name).
Comment From: gnvk
Just my 2 cents on this: in a real code base this can lead to a crash that doesn't trivially show the root cause. And the exact same code did work in go 1.22, so in my opinion something should be done about this, because technically this is a breaking change. The linked documentation
A Timer must be created with NewTimer or AfterFunc
doesn't mention that after creation you cannot copy the timer's value.
Comment From: ianlancetaylor
Unless we get a lot of reports about breakage due to this, we aren't going to change it.
Comment From: l0nax
We did run into this issue after upgrading our codebase to go 1.23.3. But using time.Ticker, not time.Timer.
The resulting panic was very misleading:
fatal error: ts set in timer
goroutine 11 gp=0xc000604540 m=3 mp=0xc0000a4e08 [running]:
runtime.throw({0xae63ed?, 0x0?})
/home/eb/.gvm/gos/go1.23.4/src/runtime/panic.go:1067 +0x48 fp=0xc0000d59a8 sp=0xc0000d5978 pc=0x474528
runtime.(*timers).addHeap(0xc000064290, 0xc000416dd0)
/home/eb/.gvm/gos/go1.23.4/src/runtime/time.go:386 +0x156 fp=0xc0000d5a00 sp=0xc0000d59a8 pc=0x45b796
runtime.(*timer).maybeAdd(0xc000416dd0)
/home/eb/.gvm/gos/go1.23.4/src/runtime/time.go:635 +0x105 fp=0xc0000d5a50 sp=0xc0000d5a00 pc=0x45c005
runtime.(*timer).modify(0xc000416dd0, 0x95664599a01, 0x3b9aca00, 0x0, {0x0, 0x0}, 0x0)
/home/eb/.gvm/gos/go1.23.4/src/runtime/time.go:572 +0x265 fp=0xc0000d5a80 sp=0xc0000d5a50 pc=0x45be25
runtime.(*timer).reset(...)
/home/eb/.gvm/gos/go1.23.4/src/runtime/time.go:649
time.resetTimer(0x3b9aca00?, 0x973?, 0xa09?)
/home/eb/.gvm/gos/go1.23.4/src/runtime/time.go:362 +0x25 fp=0xc0000d5ac8 sp=0xc0000d5a80 pc=0x478f05
time.(*Ticker).Reset(0xc000416dc0, 0x3b9aca00)
/home/eb/.gvm/gos/go1.23.4/src/time/tick.go:72 +0x3d fp=0xc0000d5af0 sp=0xc0000d5ac8 pc=0x4a2d5d
<redacted | not useful>
The timer was created as in the issue description.
From my point of view go vet should at least report something.
Comment From: l0nax
@ianlancetaylor I think the golang tools should warn users of this type of misuse. This behavior is not documented, and the resulting panic message is very confusing – especially for those new to the language.
Adding noCopy to time.Timer/ time.Ticker will not trigger a linter warning for the ticker := *time.NewTicker(time.Second) case. I'm not sure if this is intended in the vet copy lock linter or not.
Relevant issues are #32550 and #16227.
Comment From: apparentlymart
Just in the interests of connecting these two issues, note that https://github.com/golang/go/issues/70811 is currently proposing a more general solution to warning about unwise copying. If that proposal were accepted then perhaps this proposal would become a library change to add a NoCopy sigil field to the type rather than to directly change go vet.
(I'm not intending this comment as favor for one design over the other; I just want to make sure there's cross-links between these two slightly-overlapping proposals.)
Comment From: ianlancetaylor
@l0nax To be clear, I'm fine with a vet check. When I say that we aren't going to change the behavior, I mean that we aren't going to restore the 1.22 behavior, not that we aren't going to add a vet check.
Comment From: gh2o
+1000 on adding a vet check: a obj.timer = *time.NewTicker(...) line was in our codebase and worked until we upgraded to Go 1.23. The symptom was that we had a test that froze until it was signaled by SIGQUIT. It was really hard to track down the root cause because the goroutine stack didn't show what was going on either.
SIGQUIT: quit
PC=0x481241 m=0 sigcode=0
...
goroutine 171 gp=0xc000229180 m=3 mp=0xc0000dce08 [running]:
goroutine running on other thread; stack unavailable
created by testing.(*T).Run in goroutine 1
/code/go/src/testing/testing.go:1743 +0x390
...
*** Test killed with quit: ran too long (22m0s).
The goroutine above was the one that invoked the obj.timer = *time.NewTicker(...) call, and shows that we couldn't even get a stack trace of where the call occurred. We had to add a ton of print calls to figure out where the freeze occurred.
Comment From: gopherbot
Change https://go.dev/cl/676335 mentions this issue: go/analysis/passes/copylock: check copy of Timer/Ticker/etc.
Comment From: adonovan
See also: - https://github.com/golang/go/issues/73886.
Comment From: adonovan
I ran the vet check in CL https://go.dev/cl/676335 over a sample of 15K packages from the module mirror corpus and got the following results:
A random 20 of each are shown below:
file: https://go-mod-viewer.appspot.com/github.com/go-swagger/go-swagger@v0.31.0/cmd/swagger/commands/diff/checks_test.go#L171: call of isRefType copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L30: literal copies timer value from time.NewTimer(d): time.Timer https://go-mod-viewer.appspot.com/github.com/codingeasygo/util@v0.0.0-20231206062002-1ce2f004b7d9/xsql/xsql.go#L96: return copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa@v1.3.14/internal/heart/heart.go#L61: return copies timer value: github.com/diamondburned/arikawa/internal/heart.Pacemaker contains time.Ticker https://go-mod-viewer.appspot.com/google.golang.org/grpc@v1.72.2/balancer/rls/cache_test.go#L212: call of cmpopts.IgnoreUnexported copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/m-lab/locate@v0.17.6/heartbeat/heartbeat.go#L51: assignment copies timer value to ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa@v1.3.14/internal/heart/heart.go#L53: literal copies timer value from time.NewTicker(heartrate): time.Ticker https://go-mod-viewer.appspot.com/gitlab.com/ModelRocket/reno@v1.1.28-0.20201207181659-f096f7620c3b/types/ticker.go#L44: assignment copies timer value to t.ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa/v2@v2.1.0/internal/heart/heart.go#L106: assignment copies timer value to p.ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/mattn/anko@v0.1.10/packages/time.go#L70: call of reflect.TypeOf copies timer value: time.Ticker https://go-mod-viewer.appspot.com/google.golang.org/grpc@v1.72.2/balancer/rls/cache_test.go#L129: call of cmpopts.IgnoreUnexported copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/safing/portbase@v0.19.5/modules/sleepyticker.go#L18: literal copies timer value from time.NewTicker(normalDuration): time.Ticker https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L35: literal copies timer value from time.NewTicker(d): time.Ticker https://go-mod-viewer.appspot.com/github.com/go-swagger/go-swagger@v0.31.0/cmd/swagger/commands/diff/checks_test.go#L170: assignment copies timer value to ro: time.Timer https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa/v2@v2.1.0/internal/heart/heart.go#L76: return copies timer value: github.com/diamondburned/arikawa/v2/internal/heart.Pacemaker contains time.Ticker https://go-mod-viewer.appspot.com/gitlab.com/ModelRocket/reno@v1.1.28-0.20201207181659-f096f7620c3b/types/ticker.go#L36: literal copies timer value from time.NewTicker(period): time.Ticker https://go-mod-viewer.appspot.com/github.com/m-lab/locate@v0.17.6/cmd/heartbeat/main.go#L124: assignment copies timer value to hbTicker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa/v2@v2.1.0/internal/heart/heart.go#L68: literal copies timer value from time.NewTicker(heartrate): time.Ticker https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L25: literal copies timer value from *time.AfterFunc(d, f): time.Timer https://go-mod-viewer.appspot.com/github.com/simpleiot/simpleiot@v0.18.3/client/network-manager.go#L190: assignment copies timer value to syncTick: time.Ticker
timer: https://go-mod-viewer.appspot.com/gitlab.com/ModelRocket/reno@v1.1.28-0.20201207181659-f096f7620c3b/types/ticker.go#L36: literal copies timer value from time.NewTicker(period): time.Ticker https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L35: literal copies timer value from time.NewTicker(d): time.Ticker https://go-mod-viewer.appspot.com/github.com/m-lab/locate@v0.17.6/heartbeat/heartbeat.go#L51: assignment copies timer value to ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa/v2@v2.1.0/internal/heart/heart.go#L106: assignment copies timer value to p.ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/mattn/anko@v0.1.10/packages/time.go#L70: call of reflect.TypeOf copies timer value: time.Ticker https://go-mod-viewer.appspot.com/github.com/diadata-org/diadata@v1.4.593/internal/pkg/tradesBlockService/tradesBlockService.go#L160: variable declaration copies timer value to logTicker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa/v2@v2.1.0/internal/heart/heart.go#L68: literal copies timer value from time.NewTicker(heartrate): time.Ticker https://go-mod-viewer.appspot.com/github.com/go-swagger/go-swagger@v0.31.0/cmd/swagger/commands/diff/checks_test.go#L170: assignment copies timer value to ro: time.Timer https://go-mod-viewer.appspot.com/github.com/codingeasygo/util@v0.0.0-20231206062002-1ce2f004b7d9/xsql/xsql.go#L96: return copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/safing/portbase@v0.19.5/modules/sleepyticker.go#L18: literal copies timer value from time.NewTicker(normalDuration): time.Ticker https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L25: literal copies timer value from time.AfterFunc(d, f): time.Timer https://go-mod-viewer.appspot.com/github.com/go-swagger/go-swagger@v0.31.0/cmd/swagger/commands/diff/checks_test.go#L171: call of isRefType copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/m-lab/locate@v0.17.6/cmd/heartbeat/main.go#L124: assignment copies timer value to hbTicker: time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa@v1.3.14/internal/heart/heart.go#L61: return copies timer value: github.com/diamondburned/arikawa/internal/heart.Pacemaker contains time.Ticker https://go-mod-viewer.appspot.com/github.com/diamondburned/arikawa@v1.3.14/internal/heart/heart.go#L53: literal copies timer value from time.NewTicker(heartrate): time.Ticker https://go-mod-viewer.appspot.com/google.golang.org/grpc@v1.72.2/balancer/rls/cache_test.go#L129: call of cmpopts.IgnoreUnexported copies timer value: time.Timer https://go-mod-viewer.appspot.com/github.com/mixer/clock@v0.0.0-20220922162503-4933054921a2/default.go#L30: literal copies timer value from *time.NewTimer(d): time.Timer https://go-mod-viewer.appspot.com/google.golang.org/grpc@v1.72.2/balancer/rls/cache_test.go#L212: call of cmpopts.IgnoreUnexported copies timer value: time.Timer https://go-mod-viewer.appspot.com/gitlab.com/ModelRocket/reno@v1.1.28-0.20201207181659-f096f7620c3b/types/ticker.go#L44: assignment copies timer value to t.ticker: time.Ticker https://go-mod-viewer.appspot.com/github.com/simpleiot/simpleiot@v0.18.3/client/network-manager.go#L190: assignment copies timer value to syncTick: time.Ticker
@gh2o would you mind taking a look over them and making a judgment about the precision (true vs false positives). Thanks.
Comment From: adonovan
I looked over the time.Timer ones: they are almost all true bugs. My favorite is this one. I did see a couple of false positives of this form: reflect.TypeOf(time.Timer{}) and cmpopts.IgnoreTypes(...). Perhaps those two functions could be special-cased since they look only at the type, not the value, of the interface argument.
Comment From: adonovan
https://go.dev/cl/676537 adds a dynamic check for use-after-copy of time.Timer values.