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

cmd/vet: doesn't allow discarded cancel function on subcontexts #51160

Open
bryndin opened this issue Feb 12, 2022 · 2 comments
Open

cmd/vet: doesn't allow discarded cancel function on subcontexts #51160

bryndin opened this issue Feb 12, 2022 · 2 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bryndin
Copy link

bryndin commented Feb 12, 2022

What version of Go are you using (go version)?

go version go1.16.5 darwin/amd64

What did you do?

go vet fails on discarded cancel function in child context, even though the context package states parent cancellation cancels all children.

Getting the cancel function returned by context.WithDeadline should be called, not discarded, to avoid a context leak

A very simplified example

func ParentCall(ctx context.Context) {
    c, cancel := context.WithCancel(ctx)
    Call(c)
    // wait for asyncs
    cancel()
}

func Call(ctx context.Context) {
    c, _ = context.WithDeadline(ctx, getDeadline())
    asyncCall(c)
    // can't call `cancel()` here since the `asyncCall` hasn't finished yet.
}
@bryndin bryndin changed the title cmd/vet: cmd/vet: doesn't allow discarded cancel function on subcontexts Feb 12, 2022
@zpavlinovic zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Feb 15, 2022
@zpavlinovic
Copy link
Contributor

zpavlinovic commented Feb 15, 2022

In order for go vet to not report this, it would need to identify the parent context, its cancel function, and then make sure that cancel is called. This might be straightforward in this example, but things get more complicated when the call sites cannot be statically resolved. In general, go vet checkers are intra-procedural as opposed to being inter-procedural.

As mentioned here, WithDeadline, WithTimeout, WithCancel return derived Context values that can be canceled sooner than the parent Context. That is why the code should call cancel as soon as the operations running in this Context complete. Calling cancel hence is a good practice and makes the intra-procedural nature of go vet a good fit here.

In your example, we don't know when asyncCall might finish (when looking from Call), so With* is not the best option IMO. Perhaps the context passed to asyncCall should be created using context.WithValue?

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 15, 2022
@dmitshur dmitshur added this to the Backlog milestone Feb 15, 2022
@timothy-king
Copy link
Contributor

Thinking out loud a bit about this example:

  1. func Call(ctx context.Context) is exported so we cannot prove from the current package under analysis that ctx is canceled from all contexts it be be called. This is a nit-picky point but it leads into the other ones.
  2. Assuming Call -> call and make the function unexported, we would need to know all of the callers to call to cover all of the control paths. This maybe feasible for static callers, but unlikely to be feasible for dynamic callers. vet should continue to report such locations if they might be callable as if they were exported. In practice this means a highly approximate "might escape to the heap" analysis. This would include functions that are assigned to variables (f := call; f(ctx)) and methods on types that flow into an interface somewhere (including fmt functions). The methods aspect worries me as it seems quite unstable for reports. Adding a Printf and the vet reports changing is action at a distance.
  3. The property we are likely to be able to show is cancel is called on non-panicing paths. This seems fine and in line with the status quo.
  4. We can boil this example down a bit more to have no function calls:
func ParentCall2(ctx context.Context) {
   c, cancel := context.WithCancel(ctx)
   Call(c)
   c2, _ = context.WithDeadline(c, getDeadline())
   asyncCall(c2)
   // wait for asyncs
   cancel()
}

It feels like this could easily be rewritten to cancel the context coming from context.WithDeadline.

That last point suggests the original version can be rewritten to return the cancel and cancel after waiting. Whether this is an acceptable or desirable direction for vet to nudge in would likely require more details. This is the direction "context"s documentation nudges as @zpavlinovic pointed out.

Thoughts from others?

stuartnelson3 added a commit to stuartnelson3/beats that referenced this issue Feb 28, 2022
golint doesn't allow us to discard a sub-context's
cancel function. Discussion is available here:
golang/go#51160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants