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: race-like behaviour with GOFLAGS=-mod=readonly set #35933

Closed
myitcv opened this issue Dec 2, 2019 · 3 comments
Closed

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

myitcv opened this issue Dec 2, 2019 · 3 comments
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

Comments

@myitcv
Copy link
Member

myitcv commented Dec 2, 2019

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 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-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).

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

@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 2, 2019
@gopherbot gopherbot added this to the Unreleased milestone Dec 2, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 2, 2019
@stamblerre
Copy link
Contributor

stamblerre commented Dec 2, 2019

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

@myitcv
Copy link
Member Author

myitcv commented Dec 2, 2019

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 stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 3, 2019
@stamblerre
Copy link
Contributor

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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

No branches or pull requests

3 participants