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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: data race in memoize #35995

Closed
myitcv opened this issue Dec 5, 2019 · 4 comments · Fixed by myitcvforks/tools#3
Closed

x/tools/gopls: data race in memoize #35995

myitcv opened this issue Dec 5, 2019 · 4 comments · Fixed by myitcvforks/tools#3
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@myitcv
Copy link
Member

myitcv commented Dec 5, 2019

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

$ go version
go version devel +acf3ff2e8a Tue Dec 3 17:35:06 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20191113223546-95cb2a1a7eae => github.com/myitcvforks/tools v0.0.0-20191119111301-0222b4b716c6
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20191115202509-3a792d9c32b2 => github.com/myitcvforks/tools/gopls v0.0.0-20191119111301-0222b4b716c6

Note that github.com/myitcvforks/tools v0.0.0-20191119111301-0222b4b716c6 and github.com/myitcvforks/tools/gopls v0.0.0-20191119111301-0222b4b716c6 correspond to the x/tools 95cb2a1 with 80313e1 cherry picked on top. Reason being, we can't move beyond 95cb2a1 because otherwise we start tripping over the mistmatched versions problem described in #35114

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
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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-build319932305=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Was running a govim test and I spotted a data race:

WARNING: DATA RACE
Read at 0x00c00036e880 by goroutine 535:
  golang.org/x/tools/internal/memoize.(*Handle).run.func1()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:197 +0x47

Previous write at 0x00c00036e880 by goroutine 329:
  golang.org/x/tools/internal/memoize.(*Handle).run.func1()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:212 +0x143

Goroutine 535 (running) created at:
  golang.org/x/tools/internal/memoize.(*Handle).run()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:196 +0x18a
  golang.org/x/tools/internal/memoize.(*Handle).Get()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:179 +0x183
  golang.org/x/tools/internal/lsp/cache.(*parseGoHandle).Parse()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/lsp/cache/parse.go:76 +0x80
  golang.org/x/tools/internal/lsp/cache.typeCheck.func1()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/lsp/cache/check.go:257 +0xaf

Goroutine 329 (finished) created at:
  golang.org/x/tools/internal/memoize.(*Handle).run()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:196 +0x18a
  golang.org/x/tools/internal/memoize.(*Handle).Get()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:179 +0x183
  golang.org/x/tools/internal/lsp/cache.(*parseGoHandle).Parse()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/lsp/cache/parse.go:76 +0x80
  golang.org/x/tools/internal/lsp/cache.typeCheck.func1()
      /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/lsp/cache/check.go:257 +0xaf
==================
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xdf8314]

goroutine 995 [running]:
golang.org/x/tools/internal/memoize.(*Handle).run.func1(0xc00036e840, 0x11d3320, 0xc000cfcf00)
 /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:197 +0x64
created by golang.org/x/tools/internal/memoize.(*Handle).run
 /home/travis/gopath/pkg/mod/github.com/myitcvforks/tools@v0.0.0-20191119111301-0222b4b716c6/internal/memoize/memoize.go:196 +0x18b

What did you expect to see?

No data race 😄

What did you see instead?

As above

cc @stamblerre
@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Dec 5, 2019
@gopherbot gopherbot added this to the Unreleased milestone Dec 5, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 5, 2019
@stamblerre
Copy link
Contributor

Thanks for the report!

/cc @ianthehat @heschik for golang.org/x/tools/internal/memoize expertise

@myitcv
Copy link
Member Author

myitcv commented Dec 5, 2019

No problem. Note the note on version details above, however. i.e. might have been fixed subsequently:

Note that github.com/myitcvforks/tools v0.0.0-20191119111301-0222b4b716c6 and github.com/myitcvforks/tools/gopls v0.0.0-20191119111301-0222b4b716c6 correspond to the x/tools 95cb2a1 with 80313e1 cherry picked on top. Reason being, we can't move beyond 95cb2a1 because otherwise we start tripping over the mistmatched versions problem described in #35114

@heschi heschi self-assigned this Dec 5, 2019
@heschi
Copy link
Contributor

heschi commented Dec 5, 2019

Every time I make a concurrency change in response to a review comment I break something :( Fix is simple.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/210077 mentions this issue: internal/memoize: fix race on read of handle.function

myitcv pushed a commit to myitcvforks/tools that referenced this issue Dec 6, 2019
@heschi @myitcv
internal/memoize: fix race on read of handle.function
f8cfeae 
Late into CL 206879 I started nulling out a handle's function when the
handle finished running. That invalidated a previous assumption that the
field was immutable. Fix the assumption, and since the case of having
multiple computations in flight is at least a little bit possible, try
harder to avoid duplicate work.

Fixes golang/go#35995.

Change-Id: I993fbf1653a219e329bbe0d1a6e39115cce7258f
@myitcv myitcv mentioned this issue Dec 6, 2019
Merged
myitcv added a commit to govim/govim that referenced this issue Dec 6, 2019
@myitcv
deps: use cherry pick of CL 210077 for memoize race fix
5bc5d2c 
In our fork of tools at github.com/myitcvforks/tools we also cherry-pick
the commit from CL 210077 which contains a fix for the data races (and
hence CI failures) we reported as part of:

golang/go#35995
myitcv added a commit to govim/govim that referenced this issue Dec 6, 2019
@myitcv
deps: use cherry pick of CL 210077 for memoize race fix
b74225b 
In our fork of tools at github.com/myitcvforks/tools we also cherry-pick
the commit from CL 210077 which contains a fix for the data races (and
hence CI failures) we reported as part of:

golang/go#35995
@myitcv myitcv mentioned this issue Dec 6, 2019
Merged
myitcv added a commit to myitcvforks/tools that referenced this issue Dec 7, 2019
@myitcv
internal/memoize: fix race on read of handle.function (#3)
b8ce9b8 
Late into CL 206879 I started nulling out a handle's function when the
handle finished running. That invalidated a previous assumption that the
field was immutable. Fix the assumption, and since the case of having
multiple computations in flight is at least a little bit possible, try
harder to avoid duplicate work.

Fixes golang/go#35995.

Change-Id: I993fbf1653a219e329bbe0d1a6e39115cce7258f
myitcv added a commit to govim/govim that referenced this issue Dec 7, 2019
@myitcv
deps: use cherry pick of CL 210077 for memoize race fix
f261fed 
In our fork of tools at github.com/myitcvforks/tools we also cherry-pick
the commit from CL 210077 which contains a fix for the data races (and
hence CI failures) we reported as part of:

golang/go#35995
myitcv added a commit to govim/govim that referenced this issue Dec 7, 2019
@myitcv
deps: use cherry pick of CL 210077 for memoize race fix (#611)
e3249f8 
In our fork of tools at github.com/myitcvforks/tools we also cherry-pick
the commit from CL 210077 which contains a fix for the data races (and
hence CI failures) we reported as part of:

golang/go#35995
@golang golang locked and limited conversation to collaborators Dec 4, 2020
@rsc rsc unassigned heschi Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

internal/memoize: fix race on read of handle.function myitcvforks/tools
4 participants
@myitcv @stamblerre @gopherbot @heschi