Remove lsp4j on v1 branch #12
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements a stdio server, which reads JSON-RPC messages off its :in, processes them, and sends responses/notifications to :out. There's still a lot of exception handling to do, and it needs a careful comparison to the spec.
Clients have two ways to shutdown. They can send the 'shutdown' request, or they can close their end of the server input. The shutdown is mostly for the language server to do cleanup, so now we do nothing. On exit, we simulate the server input closing, which blocks waits for messages to finish being processed, then closes the output, then unblocks.
I've been having a problem with the new code that wasn't happening in lsp4j. Here's what I _think_ is going on in lsp4j. (Java code is so hard for me to read.) First we createLauncher. This instantiates several objects: 1. A RemoteEndpoint. This is the thing that will processes messages. In ring terms it is the handler. 2. A MessageConsumer. This is a list of things that will all 'consume' messages. Each one does something with a message, then passes the message to the next consumer. Technically, the RemoteEndpoint is also a MessageConsumer. It's the first in the list. The last one writes JSON-RPC to the wire. Optionally, in between, you can have tracers or other consumers. In ring terms, this is a stack of middleware that handles a request and transforms the response in several ways. 3. A StreamMessageProducer. This is the thing that will read JSON-RPC off the wire, convert it to Java obects, and call the MessageConsumer list. In ring terms, it's middleware that transforms the request before calling the handler. 4. A ConcurrentMessageProcessor. This is the thing that will connect the producer to the consumer list. Then we ask the launcher to startListening. This starts a thread in which the ConcurrentMessageProcessor will: 1. Ask the producer to start a loop that reads messages. 2. Pipe those messages to the consumer stack. Despite using newCachedThreadPool, (and despite the name "Concurrent") only [one thread is created](https://github.com/eclipse/lsp4j/blob/6e20087ec9e6201a6cdabf746c7233fe3c87abbc/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/ConcurrentMessageProcessor.java#L105-L108) to read messages. So, one background thread reads messages from the client, and forwards them to the main thread. In the main thread, lsp4j receives requests and notifications from the client, processes them (by forwarding them to clojure-lsp) and replies. On the clojure-lsp side, we start many threads and so it's possible for many of them to ask lsp4j to send messages to the client—for example, by calling publishDiagnostics—all at the same time. publishDiagnostics doesn't block waiting for the client to respond. But calling that method will result in an outbound message. All outbound messages, whether responses to client requests or server-generated requests and notifications, all go to a single object. (There's some messiness with Service objects and reflection in here that I don't really understand.) In any case, as it's writing JSON output, that object [synchronizes](https://github.com/eclipse/lsp4j/blob/6e20087ec9e6201a6cdabf746c7233fe3c87abbc/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/StreamMessageConsumer.java#L66) its writes across threads. I'm not sure how big the buffer is between the server's output and the client's input, but if the client is slow to read messages, the buffer will fill up and the server won't be able to write. So, effectively you can only write messages as fast as lsp4j can put them on the output stream. If the server-side writer can't keep up or the client-side reader is slow, publishDiagnostics will block waiting for other threads to finish writing. In the new architecture, output is modeled a bit differently. There's a channel which gets messages put on it. A go loop is reading messages off the channel, one-by-one, serializing them to JSON and writing them to the output stream. Now we're in a situation where that go block, like the synchronized lsp4j writer before it, can only write messages as fast as the client is willing to read them. If clojure-lsp creates thousands of messages to `put!` on the chan, and the go block doesn't drain the put queue fast enough, you end up with the exception about having too many pending puts. Unfortunately, I've already seen this exception in the wild. When metabase starts up, we publish thousands of diagnostics messages, one for each of the files in that very large project. The server-side writer is slow (possibly because the client-side reader is slow) and so we get the pending puts exception. I think we are _just_ over the limit. The put queue can only contain 1024 messages, but metabase has 1084 Clojure files. After some experimentation I have one way to fix it, which is this patch. In lsp4clj, in `send-notification`, I can use `>!!` instead of `put!` so that if the go block gets backed up, we block further notifications. (I tried `(go (>! output msg))` but that causes the same pending puts exception, which the core.async wiki [suggests will be the case](https://github.com/clojure/core.async/wiki/Go-Block-Best-Practices#general-advice).) After changing `send-notification` to use `>!!`, `spawn-async-tasks` can no longer use `go-loop` (because you aren't supposed to use blocking operations inside go blocks for fear of depleting all the threads that process the go blocks). So, on the clojure-lsp side, `spawn-async-tasks` has to be switched to `(thread (loop []))`. Maybe that's fine. It dedicates 4 threads to the 4 async tasks, but that's not terrible. This works, and I'd be happy with it as a long-term solution. It may even be correct... we are, after all, doing blocking io. Perhaps in a sense, we've essentially discovered a subtle bug. All along we've been doing blocking io inside a go block. We probably haven't ever depleted all the go threads, but it's been possible... and now, it's fixed! But maybe there's a way where it can all be solved on the lsp4clj side, without changing `spawn-async-tasks`. I'll think about that more.
Clients generally expect that their messages will be received and processed in order. If lsp4clj proccesses messsages in parallel, then that ordering gets lost. The client might send `textDocument/definition` followed by `textDocument/rename`, but the server could receive those messages in the opposite order. There is some flexibility in the spec about message ordering [spec-ordering]. Servers can introduce their own parallelism, but they then have to take responsibility for the consequences. For example clojure-lsp processes `textDocument/didChange` asynchronously, and then needs `process-after-changes` to restore ordering. But, lsp4clj should not force parallelism on every request, for every server. [spec-ordering]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#messageOrdering)
To avoid corrupting the output stream.
2 tasks
|
And @Cyrik—I meant to mention... when you're upgrading if you'd like a little direction, I'd be happy to chat. Perhaps we can turn our discussion into a Usage section in the README. |
|
Sorry for the noise, but I forgot our plan for how to deploy this. I'm going to merge this into a |
|
Thanks for the great work @mainej. I've been on a business trip but I'll try to take a look at this next week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces lsp4j with a Clojure implementation. The full list of dependencies is now clojure, core.async, cheshire, and camel-snake-kebab.
Also merges the lsp4clj-server and lsp4clj-protocol projects into one top-level project, lsp4clj.
clojure-lsp has an accompanying PR.
@Cyrik, as stated in the CHANGELOG, other users (you, I think) of lsp4clj are encouraged to upgrade. You can follow that clojure-lsp PR as guidance on the changes that'll need to be made. They're fairly extensive, but I believe will make lsp4clj a more stable platform for LSP development.
Notable changes:
lsp4clj.server/receive-requestandlsp4clj.server/receive-notificationmultimethods. (In contrast to the protocols based approach we had before.) This gives servers much more flexibility. They can implement new parts of the spec at their leisure, or according to the needs of their language. They can also choose when and how to implement asynchronicity. The tradeoff is that much of thespecchecking and logging moves back to clojure-lsp and other servers. The servers also have to know and use the names of the LSP requests/notifications. Personally, I findclojure-lsp.servercode to be more readable now, even though it has a few more responsibilities.Fixes #8.