Add support for client requests

[ebkalderon]

Currently, the Printer and associated MessageStream only support sending notifications from the server to the client.

This is due to both limitations in jsonrpc-core and design difficulties in making the Server and LspService bidirectional. Implementing this in a satisfactory way would allow the following server-to-client requests to be implemented:

• window/showMessageRequest
• client/registerCapability
• client/unregisterCapability
• workspace/workspaceFolders
• workspace/configuration
• workspace/applyEdit

Thankfully, these aren't quite as critical as the rest of the requests in the LSP specification, but it would still be nice to support them eventually for the sake of completeness.

[ebkalderon]

This JSON-RPC WebSocket client looks simple enough. We could maybe replace MessageStream with a similar client which implements both Sink and Stream, and Server::interleave() will interleave the requests and responses into stdout and stdin, respectively. Further investigation in this direction is still needed.

[ebkalderon]

As mentioned in the description, pull request #28 implemented a temporary hack that allows us to send requests but never read the responses, dropping them instead. Basically, we are treating them like JSON-RPC notifications for now.

This hack is similar to what RLS does to support client/registerCapability, client/unregisterCapability, and workspace/applyEdit. The first and second methods have empty response bodies, and checking the response body of the third isn't that critical for basic support.

Pull request #29 added an atomic request ID counter to Printer and added crude support for client/registerCapability and client/unregisterCapability requests. Both seem to work as expected in my editor using the coc.nvim language client.

[ebkalderon]

Pull request #33 added support for the workspace/applyEdit request. As with the previous client requests, we still rely on the same hack to make this possible.

[icsaszar]

I think a first step would be to have some data structure in Printer, like a bitset. The bit corresponding to the outgoing request number (which is produced by Printer::request_id) would be set when the request is made and would be cleared when the printer gets the response.

We also need to add a reference to the delegate in LspService so the responses can be routed to it in LspService::call.

The first problem here is that the delegate gets consumed here.

handler.extend_with(delegate.to_delegate());

We would probably need multiple references to delegate so I'm not quite sure how to go about it (the first thing would be to investigate the restrictions imposed by the jsonrcp crate).

In the worst case I think the server and client part of the internal service and delegate implementation will have to be separated, with jsonrpc handling the server (client -> server requests and notifications) as in the current implementation and have a separate client that handles server -> client requests and notifications (I would probably rename the printer to something like client).

For processing the actual responses I think the nicest API would be with callbacks, like this:

printer.configuration(params, |response| {
    let _ = response;
});

This would require storing the closures in the printer and executing them when a response arrives. I would go with defining an enum ResponseHandler with each response variant (ApplyWorkspaceEditResponseHandler, ShowMessageRequestResponseHandler, etc) and changing the bitset to a HashMap<u64, ResponseHandler> or TreeMap<u64, ResponseHandler>.

[ebkalderon]

Unfortunately, it is impossible to avoid consuming the delegate due to how jsonrpc-core fundamentally works (see paritytech/jsonrpc#487). I'm holding out for hope that bidirectional JSON-RPC over stdio will be possible with jsonrpsee, another crate which is slated to replace jsonrpc-core (which is currently in maintenance mode, so these limitations are unlikely to be fixed).

I agree that we could potentially create a router which sits in the LspService which routes incoming requests to the IoHandler and incoming responses to the Printer, aka Client. We would also need to write our own IoHandler-like data structure inside of Client which can track the request IDs (which is essentially what you are describing with the bitset approach). jsonrpc-core-client does not support a stdio transport, so creating such a router would be necessary for this to work.

I don't believe callbacks would be the ideal ergonomic choice here. I've actually managed to resolve #58 locally and am using async-trait to easily and ergonomically represent async methods, so I feel that we could make all notification methods async fn and rely on native async/await inside of these methods instead of relying on closures. I'll be opening a pull request to convert to std::future::Future and async/await very shortly. 😄

[ebkalderon]

This issue was accidentally closed by d2f5380 due to the "resolves" keyword being picked up in the commit message.

[ebkalderon]

Since #101 has migrated tower-lsp to std::future and async/await, it now should be possible for us to transform all request methods on Printer to async fn and theoretically support responding to client requests in an ergonomic manner, once we get the router situation worked out. There is no need to convert the existing notification methods on Printer to async fn since notifications are sent as one-off messages and do not need to be waited on by the server.

Since it is common for client requests to be emitted in response to notifications such as LanguageServer::did_open(), we may need to consider changing all available trait methods except for initialize() to async fn so we can potentially call .await and asynchronously receive client responses inside the method bodies.

[icsaszar]

What do you think about using another mpsc::channel in the Printer for the incoming requests that would be used in the call method of LspService?

I'm not too familiar with async rust so I can't really comment on that part for now, but this project could be a great learning opportunity.

[ebkalderon]

@icsaszar That is precisely what I was thinking of doing, albeit with an async channel instead. Still, it's important to keep in mind that this bidirectional processing of stdio requests and responses through Service::call() is something of a hack that doesn't really fit the tower model, but makes up for the fact that it's not possible to have a true bidirectional full duplex stream over stdio. If there's some way to avoid this, I'd be happy to pursue that first. But otherwise, we can go ahead and add that channel at the cost of coupling tightly to the stdiotransport.