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.

Sign up for GitHub

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

Jump to bottom

proposal: annotation to generalize lostcancel for other kinds of cleanup function #65682

Open
adonovan opened this issue Feb 12, 2024 · 8 comments
Open

proposal: annotation to generalize lostcancel for other kinds of cleanup function #65682

adonovan opened this issue Feb 12, 2024 · 8 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Milestone
Proposal

Comments

@adonovan
Copy link
Member

adonovan commented Feb 12, 2024 •
edited

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 type func(), often named something like cleanup, release, stop, close, or shutdown, 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:

package p

import "golang.org/x/tools/analysis/annotations/lostcancel"

type Resource struct { ... }

// Acquire (on success) returns a resource.
// The caller must call the release function when they are finished using the resource.
func Acquire() (*Resource, ReleaseFunc, error) { ... }

func ReleaseFunc func() 

func _() {
    lostcancel.Register(CancelFunc(nil))
}

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.

package annot // hypothetical new std annotations package

// A CleanupFunc is a function that must be called to clean up a resource.
//
// Here is the typical pattern:
//
//        resource, cleanup, err := p.Acquire()
//        if err != nil { return err }
//        defer cleanup()
//        ... use resource ... 
//
// Failure to call cleanup (or at least mention it) on all control paths not
// dominated by `err != nil` may be reported as an error by the `lostcancel` checker.
type CleanupFunc func()

Related:
@adonovan adonovan added Proposal Analysis Issues related to static analysis (vet, x/tools/go/analysis) labels Feb 12, 2024
@gopherbot gopherbot added this to the Proposal milestone Feb 12, 2024
@earthboundkid
Copy link
Contributor

Is there a reason not to do this via a magic comment? That would be my first instinct as a user. Something like //go:vet nolostcancel on the return closure type.

@jimmyfrasche
Copy link
Member

Maybe //vet:noLostCancel or go-vet: so it doesn't imply that it has runtime support?

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

@adonovan
Copy link
Member Author

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.

@jimmyfrasche
Copy link
Member

I agree in general though I'm not especially enthused by

func _() {
    lostcancel.Register(CancelFunc(nil))
}

If you could write type A[T any] = T, you could do type MyCleanupFunc annotations.CleanupFunc[func()]. That loses some of the type checking but vet could also provide an error for that.

@adonovan
Copy link
Member Author

If you could write type A[T any] = T, you could do type MyCleanupFunc annotations.CleanupFunc[func()]. That loses some of the type checking but vet could also provide an error for that.

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

@earthboundkid
Copy link
Contributor

There's already a common convention of using package level var _ = for type checks like Impl implements Interface. It would be nice to be able to fit this into that and not need an init func. Ideally, it would all compile away.

@timothy-king
Copy link
Contributor

@earthboundkid It is not clear you cannot do var _ = ... yet. I don't think the proposal specifies what lostcancel.Register(...) returns. If it returns something, this is clearly doable. It might be less clear that lostcancel.Register is a not a run time operation. Though I am bit doubtful that func _() { ... } will be obvious to most Go programmers.

@earthboundkid
Copy link
Contributor

I misread the underbar func as an init func, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
5 participants
@earthboundkid @jimmyfrasche @timothy-king @adonovan @gopherbot