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: log errors during `gopls check` #35564

russelldavis opened this issue Nov 13, 2019 · 9 comments

x/tools/gopls: log errors during `gopls check` #35564

russelldavis opened this issue Nov 13, 2019 · 9 comments


Copy link

@russelldavis russelldavis commented Nov 13, 2019

What did you do?

Trigger a simple error in gopls, for example by setting GOROOT to an invalid path:

GOROOT=/invalid gopls -rpc.trace -v check compile/main.go

What did you expect to see?

gopls should immediately exit after printing the error.

What did you see instead?

gopls hangs for 30s after printing the error, then prints gopls: timed out waiting for results from ...

I suspect the same thing is happening with the 30s delay in #35520

Build info 0.2.0 h1:ddCHfScTYOG6auAcEKXCFN5iSeKSAnYcPv+7zVJBd+U= h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ= h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ= h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= h1:FNzasIzfY1IIdyTs/+o3Qv1b7YdffPbBXyjZ5VJJdIA= h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc= h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=

Go info

go version go1.13.4 darwin/amd64

GOENV="/Users/russell/Library/Application Support/go/env"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bk/1vdjn7vd19x41x0ztsbvmw400000gq/T/go-build194598443=/tmp/go-build -gno-record-gcc-switches -fno-common"
@gopherbot gopherbot added the gopls label Nov 13, 2019
Copy link

@gopherbot gopherbot commented Nov 13, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

Copy link

@ianthehat ianthehat commented Nov 13, 2019

The trouble is the nature of diagnostics in the LSP.
They are delivered asynchronously to the client on the servers whim, and they can be delivered multiple times for any given file, changing the list of issues each time. There is no call the client can use to know if the server is done delivering diagnostics or not, which means the command line gopls has to just wait and see if it gets diagnostics for the file of interest. It currently waits for at most 30s before giving up.
So there are two issues:

  1. we try in gopls to make sure we always deliver diagnostics exactly once for any file/version of interest, which we are failing to do in this case
  2. we need a signal that no more diagnostics are pending so that the command line does not have to have a time out wait (fixing this would mean we could relax the restriction in 1)
Copy link

@stamblerre stamblerre commented Nov 13, 2019

In gopls check, we might able to handle the logging of errors in some special way to indicate that we're exiting early. What do you think, @ianthehat?

Copy link

@ianthehat ianthehat commented Nov 13, 2019

Yeah, that is kind of what I was thinking when I said some kind of signal, but I know there was also a proposal and some work on adding background task and progress monitoring to the LSP spec, if those have arrived (or are likely to in the future) we could do something cleaner than just trying to recognize a log message

Copy link

@stamblerre stamblerre commented Nov 14, 2019

Yeah, there's definitely support for in-progress replies for the majority of the requests, but I think for diagnostics there isn't anything like that (yet).

@stamblerre stamblerre changed the title gopls: 30s delay after errors instead of exiting immediately x/tools/gopls: print logged errors as part of `gopls check` Nov 14, 2019
@gopherbot gopherbot added this to the Unreleased milestone Nov 14, 2019
@gopherbot gopherbot added the Tools label Nov 14, 2019
@stamblerre stamblerre changed the title x/tools/gopls: print logged errors as part of `gopls check` x/tools/gopls: improve `gopls check` Nov 21, 2019
Copy link

@stamblerre stamblerre commented Nov 21, 2019

So, to summarize this issue, we need 2 key fixes for gopls check:

  1. Errors sent by window/logMessage should be shown to the user.
  2. There needs to be some consistent way that gopls can tell the user that it is done sending diagnostics for the file in question.

@ianthehat is going to take a look at implementing these fixes.

Copy link

@stamblerre stamblerre commented Nov 29, 2019

We are now relying on the fact that the diagnostics will come for file version 1 to determine when diagnostics are done being sent. The remaining work would be make sure that logged errors are shown to the user.

@stamblerre stamblerre changed the title x/tools/gopls: improve `gopls check` x/tools/gopls: log errors during `gopls check` Dec 4, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 4, 2019
Copy link

@pjweinbgo pjweinbgo commented Jan 7, 2020

It's worse than that. If there are just Errors, they are eaten by something and never seen by the loop checking for file diagnostics. Thinking...

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Feb 2, 2020
@stamblerre stamblerre added the NeedsFix label Feb 26, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 2, 2020
Copy link

@stamblerre stamblerre commented Apr 21, 2020

I just tried running GOPATH=/invalid gopls -rpc.trace -v check ./internal/lsp/server.go to test this out, and I saw errors being logged as expected. I think that this has been fixed by our switch to using a non-standard request (diagnoseFiles).

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