proposal: cmd/vet: warn about copying a time.Timer value

[tulir]

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.

[zigo101]

package main

import "time"

func main() {
	illegalTimerCopy := *time.NewTimer(time.Second)
	illegalTimerCopy.Stop() // block for ever
}

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

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

[timothy-king]

If we do decide to add it to vet, copylock is a natural home for implementing this (if a mildly inappropriate name).

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

[ianlancetaylor]

Unless we get a lot of reports about breakage due to this, we aren't going to change it.

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

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

[apparentlymart]

Just in the interests of connecting these two issues, note that #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.)

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