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: add support for multiple concurrent clients #34111

Closed
leitzler opened this issue Sep 5, 2019 · 69 comments
Closed

x/tools/gopls: add support for multiple concurrent clients #34111

leitzler opened this issue Sep 5, 2019 · 69 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@leitzler
Copy link
Contributor

leitzler commented Sep 5, 2019

As per discussion on Slack with @ianthehat & @myitcv this is a feature request for gopls to support multiple concurrent clients.

Vim users often have multiple instances of vim running at the same time, and starting/exiting is a natural part of the workflow. Currently there is a 1 to 1 mapping between vim and gopls (at least when using govim as plugin). It would be really useful to be able to share the same cache and avoid waiting for warmup each time.

See govim/govim#491 as well as #32750 (comment) for details.

@gopherbot gopherbot added this to the Unreleased milestone Sep 5, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Sep 5, 2019
@gopherbot
Copy link

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

@leitzler
Copy link
Contributor Author

leitzler commented Sep 5, 2019

@gopherbot, please add label FeatureRequest

@ianthehat
Copy link

In its default state gopls supports a specific header based JSON stream of the LSP on stdin /stdout. In this mode it only supports a single client as stdin/stdout cannot be multiplexed.

It also has the flags -listen and -remote, which are designed to allow it to communicate using sockets. -listen only applies to the serve verb, and causes it to listen for incoming connections rather than stdin/stdout. In this mode it supports multiple clients. The -remote flag can be supplied to any verb, and tells it how to talk to a gopls that was started with serve -listen. If you use it with serve then you have a gopls that listens on stdin/stdout and forwards commands to the remote gopls. The intent is that you use this mode from within an editor to talk to a gopls server.

The protocol spoken between a -remote and -listen gopls is not defined, and never will be, we only support it as a way of intercommunication, not as an API surface. This is because to achieve some of its goals it will have to have significant extensions to the protocol, and may mutate some of the data on the way through. Part of the reason for this is that it should be feasible to have the server run on a separate machine, and it may not have access to the same file system, or it may know the files by different absolute paths etc. These features require a reliable way of translating paths, and also the remote file extension so the true server can ask the intermediary gopls for file contents. It may also be necessary to have some forms of caching and cancelling within the intermediary.

The current state is that we use this mode only for debugging. It only gets fixed when we need to use it to debug a problem, and even then it does not get properly fixed. It does mostly work, but there are things like straight proxying of shutdown message causes the shared server to quit when any of clients does.

There are also design issues still to fix, things like should we support some kind of "discovery" protocol, should we have a mode where we start a server if one is not running but connect to it if it is, when all the clients go away should the server shut down again, how do we manage the cache so we dont explode because of lots of clients over a very long time, how do we prevent one client from starving the others, how do we manage the config and environment of the server correctly etc

@myitcv
Copy link
Member

myitcv commented Sep 5, 2019

Thanks for this detail.

The current state is that we use this mode only for debugging

Understood.

For editors like Vim, Emacs, etc, where users end up starting multiple instances on the same machine having a single instance of gopls will be a big win when it avoiding waiting for (pre-)warming of the imports cache (#34115).

Given that, do you have plans to fully support this mode?

@ianthehat
Copy link

Yes, but I have no time to do anything about it right now.
It is also valuable for people that want to run gopls from the command line if it could reuse the cached results already sitting behind their editor, or from a previous command.
Doing this well you really want to have a discovery protocol though, having to specify -remote on every command would be painful.
Ideally I would like to delete the -remote flag in the future and only support a discovery protocol!

@myitcv
Copy link
Member

myitcv commented Sep 9, 2019

Yes, but I have no time to do anything about it right now.

Ok, understood. My question was more geared towards ensuring this is something that is being considered as opposed to not.

@ianthehat
Copy link

I am just being really careful to make sure people do not think I will get round to it any time soon, I don't want someone waiting on it and getting frustrated that I am not doing it!

This is also an area where contributions would be welcome, although it would be a very high touch contribution as I have a lot to say about how it is done :)

@stamblerre stamblerre added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 9, 2019
@myitcv
Copy link
Member

myitcv commented Sep 27, 2019

Just to add another motivating factor behind this change: having a single instance will amortise the additional cost of running staticcheck on large code bases each time gopls is opened.

@findleyr
Copy link
Contributor

findleyr commented Jan 9, 2020

Just to update, since this was discussed in slack: I'm working on this now, and should have some progress soon.

@gopherbot
Copy link

Change https://golang.org/cl/214803 mentions this issue: internal/lsp: add server instance to debug info

gopherbot pushed a commit to golang/tools that referenced this issue Jan 15, 2020
When debugging multiple instances of gopls simultaneously, it is useful
to be able to inspect stateful debugging information for each server
instance, such as the location of logfiles and server startup
information.

This CL adds an additional section to the /info http handler, that
formats additional information related to the gopls instance handling
the request.

Updates golang/go#34111

Change-Id: I6cb8073800ce52b0645f1898461a19e1ac980d2b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214803
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@myitcv
Copy link
Member

myitcv commented Jan 16, 2020

@findleyr is this a v1.0.0 target of v0.3.0? Not chasing... just noting that it being worked on now probably contradicts the current "unplanned" milestone 😄

@findleyr
Copy link
Contributor

Oh, thanks. Yes, I think this can be added to the v1.0.0 milestone (the v0.3.0 feature set is pretty locked-down at the moment). I'll confirm with @stamblerre.

@gopherbot
Copy link

Change https://golang.org/cl/215739 mentions this issue: internal/lsp/cmd: add a test for client logging

@gopherbot
Copy link

Change https://golang.org/cl/215740 mentions this issue: internal/lsp: remove the Context argument from NewSession

gopherbot pushed a commit to golang/tools that referenced this issue Jan 21, 2020
The passed-in Context is not used, and creates the illusion of a startup
dependency problem: existing code is careful to pass in the context
containing the correct Client instance.

This allows passing in a source.Session, rather than a source.Cache,
into lsp server constructors.

Updates golang/go#34111

Change-Id: I081ad6fa800b846b63e04d7164577e3a32966704
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215740
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jan 22, 2020
A new test is added to verify that contextual logs are reflected back to
the LSP client. In the future when we are considering servers with
multiple clients, this test will be used to verify that client log
exporting is scoped to the specific client session.

Updates golang/go#34111.

Change-Id: I29044e5355e25b81a759d064929520345230ea82
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215739
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@findleyr
Copy link
Contributor

findleyr commented Mar 6, 2020

@myitcv you didn't miss anything: that configuration isn't yet exposed. In fact, that's the last change I have planned for this issue, at which point I think we should close this and open new, more specific issues as needed.

I'd put off exposing configuration for the automatic daemon because I wasn't sure of what I wanted to do if the daemon is already running, but with different configuration parameters.

For now I will expose just the following two pieces of configuration:
-remote.listen.timeout -- the listen.timeout that should be used by an auto-started remote
-remote.debug -- the debug address configured on the remote server

Since neither of these affect serving, I think I'm NOT going to hash them into the daemon path as I do for build ID. This means that you may ask for -remote.listen.timeout=0, but if the daemon is already currently running with the default value of 1m for -remote.listen.timeout, it will continue running with that value. You'll need to restart everything to pick up the new configuration.

It will be an error to set either of these flags without also setting -remote=auto.*.

I think that keeps things simple for now -- we can always revisit in the future if this causes problems (for example, a race between govim and ALE might cause unpredictable startup configuration).

@gopherbot
Copy link

Change https://golang.org/cl/222669 mentions this issue: internal/lsp/cmd: add a dumpsessions command

@gopherbot
Copy link

Change https://golang.org/cl/222670 mentions this issue: internal/lsp/lsprpc: expose configuration for auto-started daemon

gopherbot pushed a commit to golang/tools that referenced this issue Mar 9, 2020
When running gopls as a forwarder it attempts to forward the LSP to a
remote daemon. On posix systems, by default this uses a unix domain
socket at a predictable filesystem location.

As an extra precaution, attempt to verify that the remote socket is in
fact owned by the current user.

Also, change the default TCP listen address used on windows to bind to
localhost.

Updates golang/go#34111

Change-Id: Ib24886d290089a773851c5439586c3ddc9eb797d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222246
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/222671 mentions this issue: internal/lsp: clean up on Shutdown even if not initialized

@gopherbot
Copy link

Change https://golang.org/cl/222717 mentions this issue: gopls/doc: update daemon.md and remove 'experimental' caveats

@zikaeroh
Copy link
Contributor

One thing I've noticed after doing a couple of gopls upgrades with this enabled is that the unused process seems to use CPU when I change my project's files even when no session is attached (e.g. I ask VSC to restart the language server, I get a new gopls serve process as the build ID changed, then edit a file while watching htop).

Is gopls doing some sort of extra file watching? I had thought it was only using LSP's file watcher, but the fact that it's responding to edits for files it doesn't have "open" is strange.

@zikaeroh
Copy link
Contributor

Another thing I thought of: how does gopls in this mode determine which go binary to use? I'm guessing it inherits PATH from whichever process created it, which might be wrong for future gopls invocations. For example, if I have more than one Go version, I might set PATH accordingly, or if I'm working on the go tree itself, I might set PATH to point to that to see how things behave.

@findleyr
Copy link
Contributor

@zikaeroh that's interesting re:CPU during gopls upgrades. I would not expect that, I'll see if I can reproduce.

The gopls forwarder starts the daemon with the same path as the forwarder itself. In other words, the sidecar gopls process uses its own path when starting the daemon. We can't use gopls from PATH because may LSP clients bundle a pinned gopls binary in their directory.

@zikaeroh
Copy link
Contributor

I didn't mean which gopls binary, I meant which actual go binary it'd use via go/packages, go mod tidy, etc. I start my editor with code . which means I get whichever go is in PATH when I launch which I assume is what gopls is using. If the long running gopls server is doing the same then if I change which go I have in my PATH, the server won't see that change.

@findleyr
Copy link
Contributor

I didn't mean which gopls binary, I meant which actual go binary it'd use via go/packages

Ah, sorry I misread your comment. You are right, it's possible that a change to the go binary path won't affect the daemon. It will have indeed picked up the environment from which it was started.

Perhaps we should hash the go binary build ID into the daemon socket path, similarly to how we handle the gopls binary.

@zikaeroh
Copy link
Contributor

zikaeroh commented Mar 11, 2020

Of course, I see this becoming a slippery slope as I think there are other variables like GOPACKAGESDRIVER which can affect things too; there's probably a slew of things the existing hashing memoization takes as constant for convenience. Not to say that there isn't a line in the sand you can stop hashing at, of course... 🙂

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Mar 12, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Mar 12, 2020
Add a new toplevel `inspect` verb to the gopls command, to expose the
internal state of a running gopls server. For now, this verb exposes a
single subcommand `sessions`, which lists some information about current
server sessions on a gopls daemon.

This can be used even if the debug server is not running, which will
become the default in a later CL.

Updates golang/go#34111

Change-Id: Ib7d654a659fa47280584f9a7301b952cbccc565a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222669
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 12, 2020
Three new flags are added to the serve command, and threaded through to
the LSP forwarder:
 -remote.listen.timeout: -listen.timeout for the auto-started daemon
 -remote.debug: -debug for the auto-started daemon
 -remote.logfile: -logfile for the auto-started daemon

As part of this change, no longer enable debugging the daemon by
default.

Notably none of this configuration affects serving, so modifying this
configuration has been chosen not to change the path to the automatic
daemon. In other words, this configuration has effect only for the
forwarder process that starts the daemon: all others will connect to the
daemon and inherit whatever configuration it had at startup. This should
be OK, because in the common case this configuration should be static
across all clients (e.g., many Vim sessions all sharing the same
.vimrc).

Exposing this configuration made the signature of lsprpc.NewForwarder
a bit hard to understand, so I decided to go ahead and switch to a
variadic options pattern for initializing both the Forwarder and
StreamServer, the latter just for consistency with the Forwarder.

Updates golang/go#34111

Change-Id: Iefb71e337befe08b23e451477d19fd57e69f36c6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222670
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 12, 2020
Previously it was an error to call shutdown while the server was not yet
initialized. This can result in session leak for very short-lived
sessions.

Instead, allow shutdown to proceed and simply log the error. On the
other hand, for consistency make it an error to call initialized without
first calling initialize.

Updates golang/go#34111

Change-Id: I0330d81323ddda6beec0f6ed9eb065f8b717dea0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222671
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@zikaeroh
Copy link
Contributor

Now that this issue is closed, should I be filing new issues for the go binary and CPU usage items? Or are you already tracking them somewhere?

@findleyr
Copy link
Contributor

I'm in the process of filing them now! You're too fast!

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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

9 participants