Skip to content
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

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

Closed
matttproud opened this issue Sep 3, 2021 · 13 comments
Closed

Comments

@matttproud
Copy link
Contributor

@matttproud matttproud commented Sep 3, 2021

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
Copy link
Member

@mvdan mvdan commented Sep 3, 2021

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

For example, #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.

Loading

@matttproud
Copy link
Contributor Author

@matttproud matttproud commented Sep 3, 2021

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.

Loading

@seankhliao seankhliao changed the title vet: report time.Tick and time.NewTicker scoped to select statements proposal: cmd/vet: report time.Tick and time.NewTicker scoped to select statements Sep 3, 2021
@gopherbot gopherbot added this to the Proposal milestone Sep 3, 2021
@guodongli-google
Copy link

@guodongli-google guodongli-google commented Sep 4, 2021

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.

Loading

@josharian
Copy link
Contributor

@josharian josharian commented Sep 5, 2021

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 5, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 5, 2021

Loading

@robpike
Copy link
Contributor

@robpike robpike commented Sep 5, 2021

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.

Loading

@guodongli-google
Copy link

@guodongli-google guodongli-google commented Sep 11, 2021

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.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

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.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented Sep 15, 2021

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 #48170 (comment)?)

@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.)

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 13, 2021

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.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 13, 2021

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

Loading

@rsc rsc moved this from Incoming to Active in Proposals Oct 13, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

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

Loading

@rsc rsc moved this from Active to Likely Decline in Proposals Oct 20, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Oct 27, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants