Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functionality for composing and routing to different LSP services #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

micahscopes
Copy link

I wanted to compose two different LspService instances in my language server. I have a custom LspService implementation that routes messages to an actor as well as a trait that generates async streams from an LspRouter. Tower has a Steer utility that can be used to route messages to different services. I implemented a steer module for async-lsp which features an LspSteer service and an LspPicker trait for specifying how LspSteer should pick services.

I also added a CanHandle trait which can be implemented by LspService implementations to communicate about which messages they can field. The implementation for LspRouter simply checks to see if there's a handler registered for a given message's key.

Finally, I added a simple FirstComeFirstServe picker that uses CanHandle methods to find the first service able to handle the message.

Here's how I'm using this in my application: https://github.com/micahscopes/fe/blob/language-server-async/crates/language-server/src/server.rs#L58-L64

// ...
    let services = vec![
        BoxLspService::new(lsp_actor_service),
        BoxLspService::new(streaming_router),
    ];

    let router = LspSteer::new(services, FirstComeFirstServe);
    BoxLspService::new(router)

(If you're curious, the streaming_router is currently only used to do some chunked throttling of custom NeedsDiagnostic events that get emitted by various update-related handlers. It then emits another event FilesNeedDiagnostics which handles the actual diagnostics. I'm excited about the potential of combining streams with ordinary LspRouter handlers to get more precise control over timing and ordering of message handlers. That was the motivation for doing it this way.)

By the way, this is an awesome library, thanks for building it!

@oxalica
Copy link
Owner

oxalica commented Nov 27, 2024

I wanted to compose two different LspService instances in my language server.

What's the use case of composing multiple LspService? Is it an LSP client dispatching requests to multiple servers?

@micahscopes
Copy link
Author

micahscopes commented Nov 27, 2024

In my case the two LspServices are:

  • a custom actor-based service that I wrote myself for a variety of reasons
  • a stream generating LspRouter which doesn't have access to state, it's just used to set up flows on LSP messages

Basically I just wanted some events to be handled by one and some to be handled by the other, and the flexibility to seamlessly switch between them.

Generally the utility of this LspSteer thing should be compared to the original tower Steer. It's a pretty flexible tool and the possible use cases are beyond the scope of my imagination. Originally I thought maybe I could just use Steer itself but for reasons I can't remember at the moment I found it more straightforward to just write something like it.

@oxalica
Copy link
Owner

oxalica commented Nov 27, 2024

I just wanted some events to be handled by one and some to be handled by the other, and the flexibility to seamlessly switch between them.

I think this could be done currently using service layering, by passthrough uninterested messages? This only works for non-dynamic services and is asymmetric though.

We do not support dynamic services yet, let alone dynamic and symmetric service composing. This patch is quite big.

I'm excited about the potential of combining streams with ordinary LspRouter handlers to get more precise control over timing and ordering of message handlers.

This is an interesting benefit, but has a drawback of asking handlers one-by-one if they are interested. This is O(n) time complexity on each request or notification, comparing to the current single O(1) hash map lookup.

@micahscopes
Copy link
Author

I think this could be done currently using service layering, by passthrough uninterested messages? This only works for non-dynamic services and is asymmetric though.

That sounds quite a bit simpler, I'll try implementing something using this approach.

This is O(n) time complexity on each request or notification, comparing to the current single O(1) hash map lookup.

With n as the number of services, right? This would ideally be a pretty low number, like 3 or 4.

@micahscopes
Copy link
Author

I sketched a fallback LspService but ran into the issue that AnyEvent is not Clone, so I'm not sure how I might go about trying to handle it twice. Here's what I tried:

    fn emit(&mut self, event: AnyEvent) -> ControlFlow<Result<(), async_lsp::Error>> {
        match self.primary.emit(event.clone()) {
            ControlFlow::Continue(()) => ControlFlow::Continue(()),
            ControlFlow::Break(Err(_)) => self.fallback.emit(event),
            ControlFlow::Break(Ok(())) => ControlFlow::Break(Ok(())),
        }
    }

Is that the sort of thing you had in mind?

@oxalica
Copy link
Owner

oxalica commented Nov 28, 2024

I sketched a fallback LspService but ran into the issue that AnyEvent is not Clone

This is intended.

so I'm not sure how I might go about trying to handle it twice.

That's really an issue. Typically a middleware knows what kind of events they are interested in, so they inspect it by AnyEvent::downcast and decide whether to handle or pass-through. Like this.

For your use case, I think the solution for now would be to intrusively make one of your service a middleware layer, handle interested requests, notifications and events, and pass through all others. A generic "composer service" would be hard and still need a decider like CanHandle you proposed or tower::steer::Picker. But I'd argue it would make it more complex to implement a service.

@micahscopes
Copy link
Author

micahscopes commented Nov 29, 2024

That's really an issue. Typically a middleware knows what kind of events they are interested in, so they inspect it by AnyEvent::downcast and decide whether to handle or pass-through.

I hear you. The most useful and critical change in this whole PR is to make AnyEvent::inner_type_id public.

Actually I'm relying on this for my custom LspService to register its event handlers (Router relies on this function too). If I open another PR that just makes AnyEvent::inner_type_id public, would you be open to just that one change?

@micahscopes
Copy link
Author

I went ahead and made my language server depend on a much more minimal set of changes:

  • Making AnyEvent::inner_type_id public
  • Adding stub methods on AnyRequest and AnyNotification for writing tests

I think these changes are pretty simple, and in my view they seem pretty essential for supporting downstream LspService development.

main...micahscopes:async-lsp:pub-inner-type-id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants