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

Open
leitzler opened this issue Sep 5, 2019 · 18 comments
Open

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

leitzler opened this issue Sep 5, 2019 · 18 comments
Assignees

Comments

@leitzler
Copy link
Contributor

@leitzler 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 label Sep 5, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@leitzler leitzler commented Sep 5, 2019

@gopherbot, please add label FeatureRequest

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Sep 5, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Sep 5, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Sep 9, 2019

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

@myitcv

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 16, 2020

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 21, 2020

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 21, 2020

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

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 24, 2020

Update: I've got a number of things working locally, but some of them required significant refactoring that needs to be evaluated before merging. I thought it would be helpful to comment here describing what I'm working on, in case anyone has opinions, questions, or advice.

Goals

Here's what I’ve identified as the primary goals for the shared gopls implementation:

  1. Ease of use: from the perspective of an LSP client, using a shared gopls instance should appear identical to using a singleton gopls instance.
  2. Debuggability: each client session should be isolated in logs and metrics.
  3. Testability: gopls tests should cover both in-process and remote instances of gopls.

Ease of use

To connect to a shared gopls instance, we'll use the existing -remote flag as specified below. This will cause the gopls process invoked by the editor plugin to operate as a thin client for a separate gopls server process, forwarding LSP messages and recording metrics and logs.

  • -remote="<addr>:<port>": connect to a specific shared gopls at that address. If it’s not running, fail.
  • -remote=auto: connect to the default shared gopls. If it's not running, start it and then connect.
  • -remote=auto:<port>: connect to a shared gopls instance at localhost:<port>, or start it and then connect.
  • -remote=auto:<id>: discover a shared gopls instance identified by a non-numeric string <id>, or start it and then connect

This will allow the following usage patterns:

  • Manual management of a shared gopls instance.
  • Automatic management of a shared gopls instance.
  • Automatic management of a set of shared gopls instances identified by specific ports or IDs. This is a minor extension of the previous use cases, and will allow for scenarios where developers are using different process/mount namespaces in their workflow (thanks @myitcv for the suggestion!)

I'm not married the the -remote=auto syntax, but I think the semantics are correct: using a shared instance should be no harder than using a singleton instance.

For future discussion, I'll refer to the thin client gopls process (the one started with the -remote flag) as the forwarder gopls, and the server gopls process (the one started with the -listen flag), as the shared gopls. This is to avoid confusion around the words ‘client’ and ‘server’, which now become relative terms.

The Server Shutdown Problem

One major problem with starting a shared gopls process automatically is server shutdown: the shared gopls will be a child process of whichever forwarder gopls process started it, and will die when that forwarder process exits. For certain workflows this might be a big problem, for example users who use only short-lived vim processes. I can think of four potential solutions for this:

  1. Simply crash all forwarder gopls when the shared gopls exits. This will force the editor plugin to restart the language server, but most editor plugins already do this. This is crude but is the easiest solution to implement.
  2. Daemonize the shared gopls process, and have it shut down when there are no more forwarder gopls connected. There are various libraries that could potentially help with this (example), but it is shaping up to be a tricky solution. Not all editors will have the necessary privileges to daemonize a process.
  3. Track session information in the forwarder gopls (essentially editor buffer state), and when the shared gopls exits, have the remaining forwarder gopls’ start another one and initializes its session state.
  4. The nonsolution: don't allow --remote=auto and just force users to start the daemon themselves.

Of these, I don't think (1) or (4) are reasonable solutions in isolation. We can't expect every user to manage their own gopls daemon, and we can't expect every LSP plugin to gracefully handle the LSP server process crashing. Notably VS Code gives up if the language server crashes five times, so if a shared gopls instance is to be used by VS Code, we shouldn’t be intentionally crashing the forwarder.

(1) and (3) both result in the loss of the gopls cache, so after a restart users would have to again pay the initial price of warming the cache. On large projects this can be painful, and since users won't be aware of which forwarder owned the shared gopls, it will be confusing when this happens. However, I will note that so far while working in x/tools with a shared gopls instance, I hardly notice when it restarts.

(2) would be the ideal solution as it results in the least amount of lost state, but I think it simply won't be possible in many executing environments. I could be wrong though: I need to do more research on daemonization.

My current plan is to start by supporting (1) and (4) so that we can all begin experimenting with using a shared gopls instance, and then work on (2) or (3) (or both, or <a better idea>) toward the end of this project.

Debuggability

I'm lifting the LSP forwarding to the jsonrpc2 layer. What is currently TCP forwarding will instead be two jsonrpc2 connections talking to each other. This is done both so that we can instrument the forwarder gopls the same way we instrument the shared gopls, and so that we can insert a handshake across the jsonrpc2 stream connecting the forwarder to shared gopls, before starting to forward the LSP. In doing so, we allow the forwarder and shared gopls to exchange information that can be used in debugging. For example, the forwarder gopls can know the location of shared logs or the shared gopls debug port.

Doing this will require some refactoring of the jsonrpc2 API.

Testability

I'm going to do a bit of refactoring of internal/lsp/tests to make it easier to run tests against a remote gopls instance. Once this is done, we should be able to run both the lsp tests and cmd tests against a remote (CLI commands will also support -remote=auto).

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 24, 2020

@leitzler pointed out on slack: it would be good to also support unix domain sockets as an IPC mechanism between forwarder and shared gopls instance (thanks for the suggestion!). I agree, but I think we will always need to support TCP as well. One use case that has been discussed is running gopls in docker, in which case exposing a TCP listener is simplest.

For now, I'm going to focus on TCP. I can add support for unix domain sockets later, or perhaps it would be a good opportunity for others to contribute.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 26, 2020

@findleyr thanks for awesome write up.

Regarding "loss of the gopls cache"

In addition to the cost of re-warming the diagnostics/analysis cache, another pain point is the re-warming of the unimported cache. This can be particularly costly is you have a large module cache.

Regarding option 2

Daemonize the shared gopls process, and have it shut down when there are no more forwarder gopls connected.

Do we really want to shut it down when there are no more forwarded gopls connected? Because if I open Vim, do some work then quit, the shared gopls will be shutdown. Meaning if I re-open Vim it will need to start from cold again. Which I think defeats the point of what we're trying to solve here, unless I misunderstood? I'd say keep the shared gopls instance running forever and provide a means via a forwarder (a flag, like -stop used in conjunction with -remote) to stop the shared gopls instance.

The option space

Per our Slack chat I completely agree it's worth getting something landed so we can start playing/experimenting. I'm minded to think that option 2 is really the only solution in the medium-long term: if a user is working in an environment where we can't daemonize then I think it's probably fair to start fallback to the current, i.e. non-remote, behaviour. I don't think we'd want to not do something with daemonization simply because we can't support 100% of cases because we will get a huge return for those cases where we can (he says, selfishly 😄)

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 27, 2020

@myitcv thanks for the feedback. I think in your case the better solution might be to explicitly manage the daemon (option 4). Even if it were possible for gopls to automatically start a daemon (which won't be the case in most environments), it would be bad to silently leave behind a process that consumes so much memory.

It will always be possible to manage the daemon yourself, but it would be great if this isn't necessary for most users to get the benefit of shared state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.