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

Comments

@myitcv
Copy link
Member

@myitcv 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

@gopherbot gopherbot added this to the Unreleased milestone Feb 11, 2020
@gopherbot gopherbot added the Tools label Feb 11, 2020
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Feb 11, 2020

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

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik 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

This comment has been minimized.

Copy link
Member Author

@myitcv 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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Feb 19, 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.

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.

@stamblerre stamblerre closed this Feb 19, 2020
@leitzler

This comment has been minimized.

Copy link
Contributor

@leitzler leitzler commented Feb 19, 2020

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Feb 20, 2020

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.

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.

None yet
5 participants
You can’t perform that action at this time.