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

x/tools/gopls: False alarm with unused cancel() of derived context #69790

Closed
bronger opened this issue Oct 5, 2024 · 4 comments
Closed

x/tools/gopls: False alarm with unused cancel() of derived context #69790

bronger opened this issue Oct 5, 2024 · 4 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@bronger
Copy link

bronger commented Oct 5, 2024

gopls version

Build info

golang.org/x/tools/gopls v0.16.1
golang.org/x/tools/gopls@v0.16.1 h1:1hO/dCeUvjEYx3V0rVvCtOkwnpEpqS29paE+Jw4dcAc=
github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
golang.org/x/mod@v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
golang.org/x/sync@v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
golang.org/x/telemetry@v0.0.0-20240607193123-221703e18637 h1:3Wt8mZlbFwG8llny+t18kh7AXxyWePFycXMuVdHxnyM=
golang.org/x/text@v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
golang.org/x/tools@v0.22.1-0.20240628205440-9c895dd76b34 h1:Kd+Z5Pm6uwYx3T2KEkeHMHUMZxDPb/q6b1m+zEcy62c=
golang.org/x/vuln@v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
honnef.co/go/tools@v0.4.7 h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=
mvdan.cc/gofumpt@v0.6.0 h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo=
mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.23.0 X:rangefunc

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOEXE=''
GOEXPERIMENT='rangefunc'
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPRIVATE=''
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2627358158=/tmp/go-build -gno-record-gcc-switches'

What did you do?

package main

import (
	"context"
	"fmt"
	"time"
)

func supi(ctx context.Context) {
	var (
		innerCtx context.Context
		cancel   context.CancelFunc
	)
	ticker := time.Tick(time.Second)
	fmt.Println(innerCtx, cancel)
	for {
		select {
		case <-ticker:
			innerCtx, cancel = context.WithCancel(ctx)
		case <-ctx.Done():
			// fmt.Println(cancel)
			return
		}
	}
}

func main() {
	ctx := context.Background()
	supi(ctx)
}

What did you see happen?

The return in line 22 received the following gopls warning: “this return statement may be reached without using the cancel var defined on line 19 [default]”

What did you expect to see?

No warning, as the return can only be reached when the context is already cancelled.

Editor and settings

No response

Logs

No response

@bronger bronger added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Oct 5, 2024
@gopherbot gopherbot added this to the Unreleased milestone Oct 5, 2024
@adonovan
Copy link
Member

adonovan commented Oct 7, 2024

the return can only be reached when the context is already cancelled.

That is not in fact the case [Edit: I was wrong]. If the context passed to supi is cancelled, the return statement will be reached; its cancellation is independent of the ticker. Even if the context is never cancelled (as is the case for Background), this program is still not correct: it leaks cancellation functions at the rate of the ticker. Each call to WithCancel must be paired with exactly one call to cancel.

The analyzer is working as intended.

@adonovan adonovan closed this as completed Oct 7, 2024
@adonovan adonovan closed this as not planned Won't fix, can't repro, duplicate, stale Oct 7, 2024
@bronger
Copy link
Author

bronger commented Oct 8, 2024

Each call to WithCancel must be paired with exactly one call to cancel.

But the call to cancel happens when ctx is cancelled, doesn’t it?

@adonovan
Copy link
Member

adonovan commented Oct 8, 2024

But the call to cancel happens when ctx is cancelled, doesn’t it?

Sorry, you're quite right; I had forgotten that cancellation of the parent does release the resources of its WithCancel child contexts.

Nonetheless, this program generates a potentially unbounded number of child contexts that will never be cancelled until the parent context is cancelled (which is never for Background), so this program will indeed leak contexts at the rate of one per tick, and the analyzer is right to point out the error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants