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: 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
Assignees
Milestone

Comments

@myitcv
Copy link
Member

@myitcv 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 95cb2a1a7eae2d74d8211d4930f76e8ccd5e124f with 80313e1ba7181ff88576941b25b4beb004e348db cherry picked on top. Reason being, we can't move beyond 95cb2a1a7eae2d74d8211d4930f76e8ccd5e124f 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

@gopherbot gopherbot added this to the Unreleased milestone Dec 5, 2019
@gopherbot gopherbot added the Tools label Dec 5, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 5, 2019

Thanks for the report!

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

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv 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 95cb2a1a7eae2d74d8211d4930f76e8ccd5e124f with 80313e1ba7181ff88576941b25b4beb004e348db cherry picked on top. Reason being, we can't move beyond 95cb2a1a7eae2d74d8211d4930f76e8ccd5e124f because otherwise we start tripping over the mistmatched versions problem described in #35114

@heschik heschik self-assigned this Dec 5, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 5, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 5, 2019

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

myitcv added a commit to myitcvforks/tools that referenced this issue Dec 6, 2019
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 6, 2019
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
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 myitcvforks/tools that referenced this issue Dec 7, 2019
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.