Remove lsp4j #13
Remove lsp4j #13
Conversation
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.
| Server implementors should create `defmethod`s for the messages they want to process. (Other methods will be logged and responded to with a generic "Method not found" response.) | ||
|
|
||
| These `defmethod`s receive 3 arguments, the method name, a "context", and the `params` of the [JSON-RPC request or notification object](https://www.jsonrpc.org/specification#request_object). The keys of the params will have been converted (recursively) to kebab-case keywords. Read on for an explanation of what a "context" is and how to set it. |
There was a problem hiding this comment.
One thing I think we lost using multimethods is that we can't require that clients implement specific methods like we had on protocol, the LSP spec has some methods that all server should provide, they are required methods, like initialize, diagnostics, definition etc. I'm ok for now with this, we can think on how to make required some methods in the future
There was a problem hiding this comment.
To be pedantic, you're not required to implement every method on a protocol, the docs just say that you should. If you don't, you don't get any compile time errors, though you do get a runtime error if you try to call the method you didn't define.
But I understand what you're saying—a protocol is a stronger indication of which methods you're expected to implement.
This is just one of the drawbacks to the multimethods. They also don't give you anywhere to hang extra data about handlers. I can imagine wanting to flag certain messages as cancellable, give them validators to apply to their input or output, or more generally give them middleware style wrappers, etc.
I think the best way to think about this is that one of the typical responsibilities of a server is to route requests. The multimethods work for that, but reitit, Compojure, et al. address the fact that there's more you might want to do with routing.
On the other hand, I haven't run into anything on the clojure-lsp side yet where I really regret the simplicity of the multimethods.
So, I guess my feeling is that multimethods might not be the best design long-term, but I'm not sure I have enough information to commit to another design either.
I've vaguely considered expressing the "router" as a hashmap, keyed by method name, which you pass in to the server when you create it. The server could validate that certain keys were present. That would also give us lots of flexibility about what the values of the hashmap are. They could be functions to call as the handler (much like a multimethod, which maps dispatch values to functions). Or they could be other hashmaps, with a handler function in each. We could add other data over time. If you feel strongly that this is a better solution, I can work on it.
There was a problem hiding this comment.
To be pedantic, you're not required to implement every method on a protocol, the docs just say that you should. If you don't, you don't get any compile time errors, though you do get a runtime error if you try to call the method you didn't define.
yeah, the issue is that most clients expect that some methods are always available like diagnostics, but I agree it's best to be a little bit more loose than strict for now.
Or they could be other hashmaps, with a handler function in each. We could add other data over time. If you feel strongly that this is a better solution, I can work on it.
I think it would work well, but I'm ok trying the multimethods for now
There was a problem hiding this comment.
I'm also split on this. On the one hand, multimethods bring great simplicity, but our tooling for them is a lot worse. Previously I could find the implementation of the specific message handler with a "find reference", that's gone with multimethods. Additionally, there's no indication of all possible methods I could implement.
I still like the multimethod approach, but maybe this will drive us to add better multimethod stuff into clojure-lsp 😄
There was a problem hiding this comment.
@Cyrik we improved a lot defmethods support on clojure-lsp, doesn't find implementations work for that case?
There was a problem hiding this comment.
The thing I'm missing from defmethods is a way of seeing references to a specific method. I know that this isn't possible in the general case, which is the whole point of defmethods, but often enough there are hardcoded calls to a specific method that could be shown as a direct reference. Something like (receive-request "shutdown").
The other thing is completeness, at least in this case.
But in I also fully agree with @mainej that we should leave this as is for now since these are very minor gripes with the system.
There was a problem hiding this comment.
@Cyrik let me see if I follow...
Suppose we have this multimethod:
(defmulti caps identity)
(defmethod caps "a" [_] "A")
(defmethod caps "b" [_] "B")At call sites like the following, you know for certain that (defmethod caps "a" ...) is being called, not (defmethod caps "b" ...).
(caps "a")In theory clojure-lsp could provide navigation tools to link the a defmethod to its usage.
Of course this isn't possible when it's ambiguous which defmethod is being called:
(let [lower (rand-nth ["a" "b"])]
(caps lower))In these situations clojure-lsp couldn't help.
Assuming I'm following, I agree, that makes it harder to understand how multimethods, or really any kind of polymorphism, is being used. Polymorphism limits tooling, and that's true of all editors and languages. Maybe the next generation of tooling will offer more insight into polymorphic calls!
For these particular lsp4clj multimethods—receive-request and receive-notification—I'd say the problem isn't very big. There's only one place that calls receive-request and one that calls receive-notification (both in lsp4clj), so there's not much need to find the call sites.
I think there's a stronger argument that the other main entrypoints into lsp4clj—send-request and send-notification—exhibit a similar problem, even though they aren't multimethods. They accept the JSON-RPC method as an argument, so it could be difficult to find all the places that, for example, call (lsp.server/send-notification server "$/progress" message). In clojure-lsp we've worked around this. We have only one place that calls $/progress, a function called publish-progress. If we want to find all the call sites, we can use find-references on publish-progress. And in fact, all the calls to send-request and send-notification are in similar wrapper functions, so it's pretty easy to find references.
There was a problem hiding this comment.
@mainej Yeah that's the exact case I'm talking about. In my experience, most multimethods are built in this way (using the first param for dispatch). But all of this is very much unrelated so we should probably stop the topic 😄
To your second point: you're correct that this happens more often, I just didn't notice the change yet. Previously it was easy to find references on the ILSPProducer methods and you'll now have to do the work yourself. Maybe this could be pulled into lsp4clj as optional entry points. But I know that @ericdallo and I have differing opinions on how much lsp4clj should provide, so I'm also happy to just do that on our end. At some point, I want to publish a library with all the extra utility stuff around lsp4clj anyway (file-management, URL handling, and so on).
There was a problem hiding this comment.
we could open a new discussion on lsp4clj to see if we could provide something that help you like a different project using lsp4clj under the hood, I think discussing the problem a little bit more would make us understand better the problem
Co-authored-by: Eric Dallo <ericdallo06@hotmail.com>
This reverts commit a3595e1.
So servers can indicate that they intend to send logs and traces to the same place and want order to be preserved.
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 of lsp4clj (you, I think) are encouraged to upgrade. You can follow that clojure-lsp PR as a guide 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. When you're upgrading if you'd like a little direction, I'd be happy to chat. Perhaps we can turn our discussion into edits to the Usage section in the README.
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.