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
Comments
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. |
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. |
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.
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.
I'd recommend to include this in the DeepGo checker unless we just want to catch the "case <- ..." cases. |
cc @dominikh |
CC @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. |
If we focus on the "case" pattern, then there shall have no false positives since "time.Tick" will not be used elsewhere.
A more general checker may have false positives, e.g.
So a vet checker for the narrower cases seem reasonable to me. |
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. |
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
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 |
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. |
This proposal has been added to the active column of the proposals project |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
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:
or
Turns the patterns above occurred several hundred times. I was able to write a tool that automatically correct them safely:
That led me to assume that Go's
vet
should flag these as error prone due totime.Tick
andtime.NewTicker
being unrecoverable by the garbage collector on their own.The text was updated successfully, but these errors were encountered: