proposal: cmd/vet: report time.Tick and time.NewTicker scoped to select statements

[matttproud]

In a large codebase maintained by a mixed audience of beginner to expert developers, I did an audit for curiosity's sake on whether any production code does something tantamount to the following:

select {
case <-ch:
  // don't care
case <-time.Tick(d):
  // oops
}

or

select {
case <-ch:
  // don't care
case <-time.NewTicker(d).C:
  // oops
}

Turns the patterns above occurred several hundred times. I was able to write a tool that automatically correct them safely:

select {
case <-ch:
  // don't care
case <-time.After(d):
  // marginally better (a hot code path might want to use time.NewTimer and use the (*time.Timer).Stop method)
}

That led me to assume that Go's vet should flag these as error prone due to time.Tick and time.NewTicker being unrecoverable by the garbage collector on their own.

[mvdan]

Being the devil's advocate here for a second: perhaps a program is short-lived, and "leaking" these is not a problem.

For example, https://github.com/golang/go/issues/37681 was rejected in part because "leaks" are not always bugs, and the big ones show up in profiles so they can already be detected.

[matttproud]

Unfortunately a majority are in long-lived servers/daemons, where the functions that did this were a part of the request-scoped call graph. The release and deployment cycles are sadly not uniform either, though that is improving over time. We do see this in fleetwide profiling data.

[guodongli-google]

I think that this is a useful check. My quick search in a large code base hits around 50 matches. One question is about how much memory will leak by a Ticker: mainly a channel that usually contain one item, plus a runtimeTimer whose fields are mostly integers.

// A Ticker holds a channel that delivers ``ticks'' of a clock at intervals.
type Ticker struct {
    C <-chan Time // The channel on which the ticks are delivered.
    r runtimeTimer
}

This is actually a special case of a more general DeepGo checker that checks whether a resource is closed/terminated properly. Basically, we need to check whether Stop() is called after "ticker" is not live. More subtle cases (e.g. ticket is stopped elsewhere) will require inter-procedural analysis.

ticker := time.NewTicker(time.Second)
defer ticker.Stop()

I'd recommend to include this in the DeepGo checker unless we just want to catch the "case <- ..." cases.

[josharian]

cc @dominikh

[ianlancetaylor]

CC @robpike

[robpike]

The frequency criterion seems to be met, and maybe correctness. I am less sure about precision. False positives do seem possible, although maybe at a low enough rate and easily enough addressed not to worry.

[guodongli-google]

If we focus on the "case" pattern, then there shall have no false positives since "time.Tick" will not be used elsewhere.

case <-time.Tick(d):

A more general checker may have false positives, e.g.

t := time.Tick(d)
...

So a vet checker for the narrower cases seem reasonable to me.

[rsc]

I would rather just fix these not to be bugs. I've been meaning to do that for a long time. I am pretty sure there is an issue open for that.

[cespare]

I see two slightly different concerns at play here.

One is the problem, common to both timers and tickers, that they use memory in the runtime until they expire/are stopped (even if garbage collected). Thus, various misuses of timers and tickers cause memory leaks. This could be improved by runtime changes. (Is that what @rsc is talking about in https://github.com/golang/go/issues/48170#issuecomment-920194299?)

@ianlancetaylor has a CL from about 18 months ago which makes this improvement, but only for tickers (not timers).

The second concern, which is seemingly what this issue is focused on, is about time.Tickers that are not stopped, but are only ever received from once, as in the original example:

select {
case <-ch:
  // don't care
case <-time.Tick(d):
  // oops
}

This always seems wrong to me because it is using a ticker where the programmer clearly intended to use a timer.

IMO, if this is a mistake that people regularly make (which according to the OP it is in their codebase) then it should be flagged (by vet? by staticcheck?) whether or not the memory leak issue is tackled in some way.

(@mvdan: your devil's advocate position doesn't really make sense to me for this reason -- I can see the argument that leaking a time.After is sometimes fine, but I don't see why anyone should use a time.Ticker that they only receive from once, separate from leak concerns.)

[rsc]

I'm not sure about the details of the CL, but yes, I think something along those lines, that makes this NOT a bug, is better than vet pointing out places where there might be a bug.

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[rsc]

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

[rsc]

No change in consensus, so declined. — rsc for the proposal review group