x/tools/gopls: race-like behaviour with GOFLAGS=-mod=readonly set

[myitcv]

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

$ go version
go version devel +22688f740d Wed Nov 27 04:05:12 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-build052261892=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We have a govim test that verifies the setting of the "env" value GOFLAG=-mod=readonly. It is based on the following setup:

-- go.mod --
module mod.com

go 1.13
-- main.go --
package main

import "example.com/blah"

func main() {
	println(blah.Name)
}

(example.com/blah is a valid module and is accessible).

We initially verify that the go.mod file is not changed by gopls as we open main.go.

However we are seeing race-like behaviour from gopls. Sometimes we see an initial diagnostic for main.go (pass), sometimes not (fail).

• fail.log
• pass.log

The apparently significant line in fail.log is as follows:

Params: {"type":1,"message":"Error loading workspace folders (expected 1, got 0)\nfailed to load view for file:///tmp/go-test-script073849403/script-config_set_env_goflags_mod_readonly: error loading packages: go [list -e -json -compiled=true -test=true -export=false -deps=true -find=false -- ./...]: exit status 1: build mod.com: cannot load example.com/blah: import lookup disabled by -mod=readonly\n\n"}

This is consistent with us testing with GOFLAGS=-mod=readonly, but it does not explain why we don't see this same error consistently. Hence the thinking there is a race with some initial loading that gopls is doing?

In any case, we don't think the load via go/packages should be fatal.

What did you expect to see?

Consistently receiving an initial diagnostic from gopls for main.go

What did you see instead?

As above.

---

cc @stamblerre, @matloob

[stamblerre]

CL 209420 suppresses these errors when go/packages.Load fails. Do you still see this racy behavior with that CL?

[myitcv]

That's relatively difficult to test at this point in time.

We're looking to keep pace with the latest x/tools/gopls commits in govim/govim#584 but we're currently blocked on various issues, e.g. #35114. There are a number of govim changes involved as part of keeping pace.

This particular issue was seen in another feature branch which also has lots of changes.

Hence at this stage I can't easily confirm whether it has/hasn't fixed the issue.

[stamblerre]

Ah, I think I know what's going on here. Since we added the behavior of diagnosing all packages in a workspace on initialized, we are now racing with the type-check that also gets triggered after textDocument/didOpen. The CL mentioned above should remove the difference between the "pass" and "fail" cases, and the raciness will be mitigated by CL 209297.

I think this issue can be closed, but please open an issue if you see this again in gopls, either at master or a tagged version (I don't think we can investigate issues that happen with govim's fork of gopls, as I'm not sure how different cherrypicked CLs will interplay).