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

[bryndin]

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

[zpavlinovic]

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?

[timothy-king]

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?