all: vet context failures

[josharian]

The cmd/vet context check and the context package/methods were all added in 1.7. There are numerous complaints from vet about the stdlib. For most of them, we should probably either fix the stdlib or the vet check before release. This issue is to investigate them.

context/withtimeout_test.go:16: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak
net/http/h2_bundle.go:2000: the cancel function is not used on all paths (possible context leak)
net/http/h2_bundle.go:2005: this return statement may be reached without using the cancel var defined on line 2000
net/http/server.go:778: the cancelCtx function is not used on all paths (possible context leak)
net/http/server.go:787: this return statement may be reached without using the cancelCtx var defined on line 778

cc @bradfitz @alandonovan

[bradfitz]

Only the first looks real. We should fix ExampleWithTimeout to not be a bad example of what not to do.

The http ones are bogus.

[adonovan]

The HTTP ones are due to a bug in the vet check: a "naked" return should count as a reference to the named result variables.

[ianlancetaylor]

@adonovan do you think you can fix the vet check by the end of the week?

[adonovan]

Yes.

[gopherbot]

CL https://golang.org/cl/24681 mentions this issue.

[alandonovan]

(Not fixed yet --- https://go-review.googlesource.com/c/24681/ still pending.)

[josharian]

This doesn't look entirely fixed. At 54b499e, running go vet net/http yields:

net/http/server.go:778: the cancelCtx function is not used on all paths (possible context leak)
net/http/server.go:787: this return statement may be reached without using the cancelCtx var defined on line 778

[bradfitz]

Okay, those two are valid. Sent https://go-review.googlesource.com/24850

I disagree with these two:

transfer.go:217: net/http/internal.FlushAfterChunkWriter composite literal uses unkeyed fields
exit status 1
transport_test.go:120: arg c for printf verb %p of wrong type: net.Conn

Both of those are intentional. For the first, I think cmd/vet should ignore that rule on internal packages. And for the second, I think we need to define in the fmt package what %p means on interfaces.

[josharian]

I think cmd/vet should ignore that rule on internal packages.

I have a CL pending for 1.8 that changes transfer.go. It's a tiny change that's not obviously better or worse, and I'm unconvinced that changing vet is the right answer. But let's discuss that on the CL or during 1.8. The only thing that matters for 1.7 is the context stuff.