We had a deadlock in cases where a request was cancelled (1) after being
written to the stream, but (2) before a response was received. This
resulted in the request ID being removed from the pending map while the
server has the request, after which point the server response would hang
in Conn.Run trying to send to a nil channel.
After fixing this nil send there was still a race: it was possible that
Conn.Run could get the pending request, and Conn.Call would select
ctx.Done before Conn.Run could send to the response channel, again
resulting in a blocking send. Fix this by adding a buffer to the
response channel.
The response channel management is also made less forgiving, because we
should be able to reason precisely about how many sends and receives
will occur:
+ Don't close the response channel after sending a response: there
should only be one recipient.
+ Don't delete the ID from pending map twice: it should only be cleaned
up by Conn.Call.
Cancellation tests in the lsprpc package are updated to exercise the
race conditions.
Fixesgolang/go#37159
Change-Id: Ie3207442ea910f79247b18d8647fd52f39fb15db
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219126
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Add a new Forwarder type to the lsprpc package, which implements the
jsonrpc2.StreamServer interface. This will be used to establish some
parity in the implementation of shared and singleton gopls servers.
Much more testing is needed, as is handling for the many edge cases
around forwarding the LSP, but since this is functionally equivalent to
TCP forwarding (and the -remote flag was already broken), I went ahead
and used the Forwarder to replace the forward method in the serve
command. This means that we can now use the combination of -listen and
-remote to chain together gopls servers... not that there's any reason
to do this.
Also, wrap the new regression tests with a focus on expressiveness when
testing the happy path, as well as parameterizing them so that they can
be run against different client/server execution environments. This
started to be sizable enough to warrant moving them to a separate
regtest package. The lsprpc package tests will instead focus on unit
testing the client-server binding logic.
Updates golang/go#36879
Updates golang/go#34111
Change-Id: Ib98131a58aabc69299845d2ecefceccfc1199574
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218698
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Add a test for the bug reported in golang/go#37049: we are missing empty
diagnostics for deleted files. Doing this involved added a missing
RemoveFile method on the fake.Watcher type.
Skip the test for now, as it is failing.
Updates golang/go#37049
Updates golang/go#36879
Change-Id: Ib3b6907455cc44a2e6af00c2254aa444e9480749
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218278
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
A lot of bug reports originating from LSP clients are related to either
the timing or sequence of editor interactions with gopls (or at least
they're originally reported this way). For example: "when I open a
package and then create a new file, I lose diagnostics for existing
files". These conditions are often hard to reproduce, and to isolate as
either a gopls bug or a bug in the editor.
Right now we're relying on govim integration tests to catch these
regressions, but it's important to also have a testing framework that
can exercise this functionality in-process. As a starting point this CL
adds test fakes that implement a high level API for scripting editor
interactions. A fake workspace can be used to sandbox file operations; a
fake editor provides an interface for text editing operations; a fake
LSP client can be used to connect the fake editor to a gopls instance.
Some tests are added to the lsprpc package to demonstrate the API.
The primary goal of these fakes should be to simulate an client that
complies to the LSP spec. Put another way: if we have a bug report that
we can't reproduce with our regression tests, it should either be a bug
in our test fakes or a bug in the LSP client originating the report.
I did my best to comply with the spec in this implementation, but it
will certainly develop as we write more tests. We will also need to add
to the editor API in the future for testing more language features.
Updates golang/go#36879
Updates golang/go#34111
Change-Id: Ib81188683a7066184b8a254275ed5525191a2d68
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217598
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Previously, the process of instantiating and running the LSP server was
sharded across the lsp, protocol, and cmd packages, and this resulted in
some APIs that are hard to work with. For example, it's hard to guess
the difference between lsp.NewClientServer, lsp.NewServer,
protocol.NewServer (which returns a client), and protocol.NewClient
(which returns a server).
This change reorganizes Server instantiation as follows:
+ The lsp.Server is now purely an implementation of the protocol.Server
interface. It is no longer responsible for installing itself into the
jsonrpc2 Stream, nor for running itself.
+ A new package 'lsprpc' is added, to implement the logic of binding an
incoming connection to an LSP server session. This is put in a
separate package for lack of a clear home: it didn't really
philosophically belong in any of the lsp, cmd, or protocol packages.
We can perhaps move it to cmd in the future, but I'd like to keep it
as a separate package while I develop request forwarding.
simplified import graph:
jsonrpc2 ⭠ lsprpc ⭠ cmd
⭩ ⭦
lsp (t.b.d. client tests)
⭩ ⭨
protocol source
+ The jsonrpc2 package is extended to have a minimal API for running a
'StreamServer': something analogous to an HTTP server that listens
for new connections and delegates to a handler (but we couldn't use
the word 'Handler' for this delegate as it was already taken).
After these changes, I hope that the concerns of "serving the LSP",
"serving jsonrpc2", and "installing the LSP on jsonrpc2" are more
logically organized, though one legitimate criticism is that the word
'Server' is still heavily overloaded.
This change prepares a subsequent change which hijacks the jsonrpc2
connection when forwarding messages to a shared gopls instance.
To test this change, the following improvements are made:
+ A servertest package is added to make it easier to run a test against
an in-process jsonrpc2 server. For now, this uses TCP but it could
easily be modified to use io.Pipe.
+ cmd tests are updated to use the servertest package. Unfortunately it
wasn't yet possible to eliminate the concept of `remote=internal` in
favor of just using multiple sessions, because view initialization
involves calling both `go env` and `packages.Load`, which slow down
session startup significantly. See also golang.org/issue/35968.
Instead, the syntax for `-remote=internal` is modified to be
`-remote=internal@127.0.0.1:12345`.
+ An additional test for request cancellation is added for the
sessionserver package. This test uncovered a bug: when calling
Canceller.Cancel, we were using id rather than &id, which resulted in
incorrect json serialization (as only the pointer receiver implements
the json.Marshaller interface).
Updates golang/go#34111
Change-Id: I75c219df634348cdf53a9e57839b98588311a9ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215742
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>
