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: initialize an external context is regarded as context leak? #31856

Closed
SilverRainZ opened this issue May 6, 2019 · 6 comments
Closed

cmd/vet: initialize an external context is regarded as context leak? #31856

SilverRainZ opened this issue May 6, 2019 · 6 comments

Comments

@SilverRainZ
Copy link
Contributor

@SilverRainZ SilverRainZ commented May 6, 2019

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

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/la/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/la/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/la/git/tools/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build166721615=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Variables context, cancel are declared at toplevel, function Foo is used to to initialize them,
Indeed this code is quite poor, but is it really fine to be regard as an error?

https://play.golang.org/p/fHQ_fEu0GLW

What did you expect to see?

go vet reports nothing.

What did you see instead?

go vet reports lostcancel error:

/home/la/git/tools/main.go:11:2: the cancel function is not used on all paths (possible context leak)
/home/la/git/tools/main.go:12:1: this return statement may be reached without using the cancel var defined on line 11
@SilverRainZ SilverRainZ changed the title cmd/vet: initialize an external context is regared as context leak? cmd/vet: initialize an external context is regarded as context leak? May 6, 2019
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented May 6, 2019

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented May 6, 2019

Seems like a bug in the analyzer. If the cancel variable has package scope (x.Parent() == x.Pkg().Scope()) it should suppress the finding.

@SilverRainZ

This comment has been minimized.

Copy link
Contributor Author

@SilverRainZ SilverRainZ commented May 7, 2019

@alandonovan (x.Parent() == x.Pkg().Scope()) is just a special case for the aboved case. In my opinion, the fundamental reason is that lostcancel should skip the analysis for cancel which defined outside current function because the analysis is limited to function scope, what do you think?

This case also complained by go vet, but looks more reasonable:

// Test for Go issue 31856.
func _() {
	var cancel func()

	func() {
		_, cancel = context.WithCancel(bg)
	}()

	cancel()
}

I have filed a PR, should fix this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 7, 2019

Change https://golang.org/cl/175617 mentions this issue: lostcancel: do not analyze cancel variable which defined outside current function scope

gopherbot pushed a commit to golang/tools that referenced this issue May 8, 2019
…ent function scope

See golang/go#31856.

Change-Id: I229a7f4a48e7806df62941f801302b6da8a0c12b
GitHub-Last-Rev: 33f8523
GitHub-Pull-Request: #95
Reviewed-on: https://go-review.googlesource.com/c/tools/+/175617
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@andybons andybons added this to the Unplanned milestone May 8, 2019
@SilverRainZ

This comment has been minimized.

Copy link
Contributor Author

@SilverRainZ SilverRainZ commented May 10, 2019

@andybons The related PR is merged, this issue shouldn't be labled as NeedsInvestigation and may can be closed.

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented May 10, 2019

In future, you can write "Fixes golang/go#xxxx" if you want CLs to automatically close issues when they are merged.

@agnivade agnivade closed this May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.