ebkalderon / tower-lsp

Language Server Protocol implementation written in Rust
Apache License 2.0
951 stars 54 forks source link

Layer example does not work? #398

Closed zbirenbaum closed 10 months ago

zbirenbaum commented 10 months ago

I have been trying to figure out a way to implement a very niche use case requirement for almost two weeks and I think layers are the best way to do it, but I cannot seem to get one working. The example for timeout in layers does not seem to work, either that or I am not integrating it correctly, but can't seem to find any documentation on how to do so.

Could anyone provide some clarity on how to insert custom middleware between requests from client -> server? The use case for it is to modify a method callback so I can provide the request id or a cancellation token to a custom server method

ebkalderon commented 10 months ago

Hello there! Sorry to hear you've been running into issues. Because both the LspService (handles client-to-server requests) and the Client service (handles server-to-client requests) implement the tower_service::Service trait, the use of standard tower layers should work out of the box, with no specific customizations necessary. If this isn't the case, that's certainly not good; this would imply that there's a regression somewhere.

Would you happen to have a simplified example you can share that can be used to reproduce the problem? Ideally as a link to an external repo, but pasted inline into a comment reply would be fine. I'll try to take a look later today or tomorrow, when I have time.

zbirenbaum commented 10 months ago

Hello there! Sorry to hear you've been running into issues. Because both the LspService (handles client-to-server requests) and the Client service (handles server-to-client requests) implement the tower_service::Service trait, the use of standard tower layers should work out of the box, with no specific customizations necessary. If this isn't the case, that's certainly not good; this would imply that there's a regression somewhere.

Would you happen to have a simplified example you can share that can be used to reproduce the problem? Ideally as a link to an external repo, but pasted inline into a comment reply would be fine. I'll try to take a look later today or tomorrow, when I have time.

Thanks for the response! The Normal Exit and Initialization layers all work fine, I looked into it some more and the problem wasn't really with the Timeout layer (which isn't implemented by default, it's just an example in the comments), but rather that there's no exposure of layers to the end user. What I mean is that I can't find a way (exposed or otherwise) of easily defining and integrating a new one.

On router (which I believed isn't exposed), the method function takes three arguments, a name, a callback, and a layer. The way to add a custom handler on the server is through custom_method which takes only a name and a callback, and does not allow the addition of middleware.

Have I missed something and there is a way of doing this? If not, would it be possible to explore finding a good way to expose it?

ebkalderon commented 10 months ago

Thanks for the clarification! At the time of writing, one should be able to apply layers to the entire LspService the "standard Tower way" by way of the Layer::layer() method, like so:

+ use tower::Layer;
+
  let (service, socket) = LspService::new(|client| Backend { client });
+ let timeout_layer = /* Instantiate your layer here. */
  Server::new(stdin, stdout, socket)
      .custom_method("foo/bar", ...)
-     .serve(service)
+     .serve(timeout_layer.layer(service))
      .await;

In theory, this should be sufficient to apply the tower::timeout middleware to a tower-lsp project.

Note: if you would like to have the timeout apply only to a specific LspService method and not others, this is not exactly supported at the moment by the current custom_method builder API. Custom methods are not currently exposed as a full Tower service in its own right, since this would greatly increase the complexity of the API surface, but this also means there unfortunately is no public-facing point against which a Layer can be applied. Perhaps this choice should be re-evaluated, if enough folks demand it.

For the time being, that particular use case will require writing a custom Layer whose service wraps the entire LspService and inspects the method string of each incoming JSON-RPC Request to see if it matches a pattern. If the pattern is matched, a timeout is enforced on the request in a similar manner as the tower::timeout layer; otherwise, the inner service gets called without a timeout.

Please see the upstream documentation for the tower::Layer trait for details. The docs also include some helpful code samples indicating how to write custom layers.

zbirenbaum commented 10 months ago

Thanks for the clarification! At the time of writing, one should be able to apply layers to the entire LspService the "standard Tower way" by way of the Layer::layer() method, like so:

+ use tower::Layer;
+
  let (service, socket) = LspService::new(|client| Backend { client });
+ let timeout_layer = /* Instantiate your layer here. */
  Server::new(stdin, stdout, socket)
      .custom_method("foo/bar", ...)
-     .serve(service)
+     .serve(timeout_layer.layer(service))
      .await;

In theory, this should be sufficient to apply the tower::timeout middleware to a tower-lsp project.

Note: if you would like to have the timeout apply only to a specific LspService method and not others, this is not exactly supported at the moment by the current custom_method builder API. Custom methods are not currently exposed as a full Tower service in its own right, since this would greatly increase the complexity of the API surface, but this also means there unfortunately is no public-facing point against which a Layer can be applied. Perhaps this choice should be re-evaluated, if enough folks demand it.

For the time being, that particular use case will require writing a custom Layer whose service wraps the entire LspService and inspects the method string of each incoming JSON-RPC Request to see if it matches a pattern. If the pattern is matched, a timeout is enforced on the request in a similar manner as the tower::timeout layer; otherwise, the inner service gets called without a timeout.

Please see the upstream documentation for the tower::Layer trait for details. The docs also include some helpful code samples indicating how to write custom layers.

Thank you so much! This is perfect and much better than the way I hacked together, really appreciate the detailed response and all your work on this project!

ebkalderon commented 10 months ago

No worries! I'm really glad this info was helpful to you. I'm fully aware that the structure of tower-lsp is a bit wonky at the moment, compared to other available tower libs, mostly due to quirks of JSON-RPC and LSP that don't neatly fit into the tower model without some fiddling. Expect further improvements to come in the future, though progress may come in fits and bursts as I work this project into my regular schedule. Feel free to reopen this ticket again, if you find there still are outstanding issues remaining!