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

Jump to bottom

x/tools/gopls: Load concurrency error reported to LSP client #37164

Closed
myitcv opened this issue Feb 11, 2020 · 6 comments
Closed

x/tools/gopls: Load concurrency error reported to LSP client #37164

myitcv opened this issue Feb 11, 2020 · 6 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
gopls/v0.4.0

Comments

@myitcv
Copy link
Member

myitcv commented Feb 11, 2020

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

$ go version
go version devel +b7689f5aa3 Fri Jan 31 06:02:00 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200207224406-61798d64f025
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200207224406-61798d64f025

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-build541509370=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is a follow up to #36772 (comment)

In #36772 we encountered a number of errors, one of which was caused by two runs of cmd/go resulting in racey reads/writes to go.mod.

This is a followup issue to record a further error we are seeing be reported:

Params: {"type":1,"message":"2020/02/11 06:58:10 Load concurrency error, will retry serially: go [list -f {{context.GOARCH}} {{context.Compiler}} -- unsafe]: exit status 1: go: updates to go.mod needed, but contents have changed\n"}

gopls log file: gopls.log

What did you expect to see?

No error level log message; gopls did not fail here.

What did you see instead?

Per above.

cc @stamblerre @heschik

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

@heschik: I'll leave it up to you to decide what you want to log here. It seems fine to me to log an error, so I'm not sure that this requires a change.

@heschi
Copy link
Contributor

heschi commented Feb 11, 2020

I would be happy to make it a warning but we don't have those yet.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Feb 11, 2020
@myitcv
Copy link
Member Author

myitcv commented Feb 13, 2020

My understanding of an error being logged from server to client is that the client/user should do something with that error, i.e. investigate/report it. Hence why I'm pushing to try and eliminate all errors that are not actually errors, as well as having generic assertions in tests that verify we haven't seen any errors in the course of normal behaviour. However, if this understanding is wrong, please correct me!

It seems fine to me to log an error

Is gopls able to recover from this situation?

If it can, then I don't think this should be reported as an error.

If it can't, what are we expecting the client/user to do with the error?

cc @findleyr given discussions about govim tests.

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Feb 13, 2020
@stamblerre
Copy link
Contributor

My understanding of an error being logged from server to client is that the client/user should do something with that error, i.e. investigate/report it.

I'm not sure that I agree. To me, there is a distinction between a logged message (window/logMessage) versus a shown message (window/showMessage), and the client should only worry about messages that we show the user.

Is gopls able to recover from this situation?
If it can, then I don't think this should be reported as an error.

If gopls can't recover from an error, I think that we should show an error to the user and shut down the server. Having errors in the log helps us with troubleshooting and distinguishes informational messages from genuine error reports. We can aim to decrease the number of logged error messages, but I don't think that we can ever guarantee that zero error messages will be logged. In this case, an error has occurred and we log to indicate that we are attempting to recover from it.

I'm going to go ahead and close this issue. Please re-open if you have further thoughts, but I don't think it's incorrect for us to log the error here.

@leitzler
Copy link
Contributor

What I would expect, and I could be totally wrong, is that a log message that tells me:

Failed to xyz, will retry automatically

Is, at most, a warning. As opposite to:

Failed to xyz, won't retry (max retries exceeded for example)

That is an error.

The logMessage intention is to tell the client to log something, and if the log did contain a lot of errors that I as a user was meant to ignore, then I'd stop looking at the log. In my experience severities are a nice way to get the users attention. A retry isn't something that always needs my attention if it succeeds the second time.

I'm not saying that we should re-open the issue, just wanted to share my thoughts.

@stamblerre
Copy link
Contributor

I agree that this particular error message could probably be a warning. I've updated #36880 to reflect that we should support warnings in logs. I just want to discourage relying on the contents of gopls logs for tests.

@myitcv myitcv mentioned this issue Jul 31, 2020
Merged
myitcv added a commit to govim/govim that referenced this issue Jul 31, 2020
@myitcv
cmd/govim: change test skip to be based on golang.org/issues/36880
a4fe206 
golang.org/issues/37164 has now been closed and the outstanding issues
described in:

golang/go#37164 (comment)

have been transferred to golang.org/issues/36880.

Update the skip for the scenario_bugs/bug_gopls_37164 test to reference
golang.org/issues/36880 therefore.
myitcv added a commit to govim/govim that referenced this issue Jul 31, 2020
@myitcv
cmd/govim: change test skip to be based on golang.org/issues/36880 (#919
a26a3bf 
)

golang.org/issues/37164 has now been closed and the outstanding issues
described in:

golang/go#37164 (comment)

have been transferred to golang.org/issues/36880.

Update the skip for the scenario_bugs/bug_gopls_37164 test to reference
golang.org/issues/36880 therefore.
@golang golang locked and limited conversation to collaborators Feb 19, 2021
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
gopls/v0.4.0
Development

No branches or pull requests
5 participants
@leitzler @myitcv @stamblerre @gopherbot @heschi