-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: annotation to generalize lostcancel
for other kinds of cleanup function
#65682
Comments
Is there a reason not to do this via a magic comment? That would be my first instinct as a user. Something like |
Maybe Tangentially, there doesn't seem to be an API for getting these special comments anywhere that I can find. It would be nice to not have to parse them out yourself. The link to the NoCopy issue does not work. It looks like that was #8005 |
There are many reasons to prefer annotations over magic comments. The primary one is that they are checked by the compiler and type checker, so they cannot be misspelled, or lost during migrations, renamings, etc. In addition they are self-documenting, since you can jump to their definition, and enumerable using your IDE's references query, making it easy to find examples of use. |
I agree in general though I'm not especially enthused by func _() {
lostcancel.Register(CancelFunc(nil))
} If you could write |
That's a neat way to avoid an extra line of declaration, but I don't think the cleanup func actually needs to vary its function type (if that were even expressible). I'm open to suggestions of better ways to write a no-op Go declaration to express "type T is special". |
There's already a common convention of using package level |
@earthboundkid It is not clear you cannot do |
I misread the underbar func as an init func, for example. |
Proposal Details
The context.WithCancel function returns a "cancel" function that must be called on all execution paths. It's easy to forget to do so, especially in early-return error paths, leading to a context leak. This pattern is quite common: the gopls codebase has many functions that return a
release
function that decrements a reference count [example], and we've had a number of bugs from failure to follow the proper discipline. Kubernetes has many of its own too. Searching for functions that return a value of typefunc()
, often named something likecleanup
,release
,stop
,close
, orshutdown
, it's easy to find more instances of this pattern.Go vet currently has a lostcancel analyzer that reports problems of this sort, and it is not hard to generalize it to handle other functions besides
context.WithCancel
. (I implemented it over the weekend.) But the hardest part of the problem is reliably identifying which functions are cleanup functions, and which don't exactly follow the discipline [example]. The name is not a reliable clue, and many functions don't name their result variables.The actual type of the function returned by WithCancel is
type CancelFunc func()
, a named type. This suggests an approach to generalization: allow modules to define their own clean-up function types analogous to CancelFunc, and then register them with the analyzer using the annotation mechanism that we plan to design and develop this year.Here's a sketch of what that might look like:
This proposal is obviously subordinate to the annotations proposal, but one immediate question is: should the standard library provide a standard CancelFunc? Long time viewers may remember a similar discussion around a standard NoCopy annotation, which ended in a "no" decision. But as we think about generalizing the patterns of vet checking beyond the standard library, we may want to revisit whether the standard library should provide a lightweight package for declaring the most important annotations. This might have benefits in uniformity and boilerplate reduction, and would also allow the standard library to use the annotations, which (if they lived alongside the analyzers) would otherwise be unavailable to it.
Related:
The text was updated successfully, but these errors were encountered: