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

Receive a custom request from client #256

Closed
IWANABETHATGUY opened this issue Feb 2, 2021 · 6 comments · Fixed by #313
Closed

Receive a custom request from client #256

IWANABETHATGUY opened this issue Feb 2, 2021 · 6 comments · Fixed by #313
Assignees
Labels
enhancement New feature or request

Comments

@IWANABETHATGUY
Copy link
Contributor

Is there any way to receive a custom request from client to server?

@ebkalderon ebkalderon added the enhancement New feature or request label May 19, 2021
@ebkalderon
Copy link
Owner

Good question, @IWANABETHATGUY! There isn't currently a way to do this, but we should be able to easily expose a send_custom_request() method of some sort, similar in approach to #230.

@IWANABETHATGUY
Copy link
Contributor Author

Thank you!

@ebkalderon
Copy link
Owner

ebkalderon commented May 20, 2021

Actually, I think I might've initially misunderstood your comment. I now realize that you're referring to custom requests sent from the client to the server, rather than custom requests sent from the server to the client. That's actually a totally separate concern from #230, which is something that we also should do regardless (update: just merged #275). Sorry for conflating the two. 😄

I think this task is a bit trickier to achieve while also keeping the API ergonomic and difficult to misuse. The easy way to implement this would be to expose a fall-through handler in the LanguageServer trait for catching requests with unrecognized method names. It seems that lspower has adopted a similar solution. This handler might have a provided default body that simply returns Err(jsonrpc::Error::method_not_found()), and the user can override it to behave as they like.

This works well enough, but it feels somewhat brittle to me. It would mean that the catch-all error handling logic in tower-lsp-macros, which was carefully defined to adhere to the JSON-RPC 2.0 spec, would now be effectively user-controlled. I would honestly be more comfortable with a closure registration approach similar to the jsonrpc-core crate, if at all possible. This approach makes the API a lot harder to abuse and ensures consistent JSON-RPC behavior for custom LSP requests, without the user having to wire it up themselves.

The real question becomes, then, how to make the closure registration approach more ergonomic to the user. Prior to our switch away from jsonrpc-core in tower-lsp, we used to allow custom request/notification registration through the LspService::with_handler() method (history). Still, I don't think it was ever quite useful to anyone, though, because custom requests couldn't have access to Client nor the internal server state. If we wanted to do this right, we would have to design it a slightly different way that's state-aware. For example, would could try exposing some builder methods on LspService like these:

let (service, messages) = LspService::with_custom(|client| MyServer { client, ... })
    .request("custom/request", |server, params| server.my_custom_request(params))
    .notification("custom/notification", |server, params| server.my_custom_notification(params))
    .build();

The signature for custom request handlers would probably be something like:

async fn(Arc<MyServer>, Option<serde_json::Value>) -> tower_lsp::Result<Option<Value>>

and the signature for notification handlers could then be:

async fn(Arc<MyServer>, Option<serde_json::Value>)

I think there exist ways to reduce boilerplate even further while reducing the potential for bugs. There may also be other viable approaches out there that haven't yet been explored. In any case, hopefully we can eventually arrive at a solid failure-proof solution!

Any thoughts on this, @IWANABETHATGUY?

@ebkalderon
Copy link
Owner

So, I've implemented a basic working version of this API, which looks like this:

struct Backend {
    client: Client,
}

impl LanguageServer for Backend {
    // ...
}

impl Backend {
    async fn custom_request(&self, params: SomethingDeserializable) -> jsonrpc::Result<SomethingSerializable> {
        Ok(...)
    }

    async fn custom_notification(&self, params: SomethingDeserializable) {
        // Lack of return value indicates that this is a notification.
    }

    async fn no_params_works_too(&self) -> jsonrpc::Result<()> {
        Ok(())
    }
}

#[tokio::main]
async fn main() {
    let stdin = tokio::io::stdin();
    let stdout = tokio::io::stdout();

    let (service, messages) = LspService::builder(|client| Backend { client })
        .with_method("custom/request", Backend::custom_request)
        .with_method("custom/notification", Backend::custom_notification)
        .with_method("custom/noParamsWorksToo", Backend::no_params_works_too)
        .finish();

    Server::new(stdin, stdout)
        .interleave(messages)
        .serve(service)
        .await;
}

Underpinning this new API is a Router struct contained within the LspService which unifies all the built-in and user-defined JSON-RPC requests/notifications into a single lookup table, allowing for ergonomic and type-safe custom client-to-server requests.

In the process of designing this, though, I've encountered and reported dtolnay/async-trait#167, which appears to be caused by a known compiler bug. As a result, the builder currently cannot accept async-trait methods as callbacks. However, regular async fn methods will work just fine. Is this a significant limitation for you, @IWANABETHATGUY?

I'm inclined toward releasing this solution as-is, provided that this limitation is clearly documented. Once the rustc bug is resolved or a specific workaround is found, we can lift this restriction and permit async-trait methods to be used as well.

@IWANABETHATGUY
Copy link
Contributor Author

@ebkalderon This works for me, thank you for your work!

@ebkalderon
Copy link
Owner

I'm in the process of transplanting the quick-and-dirty prototype I created into the codebase and making it more robust. The current work is in the support-custom-server-requests branch, if you'd like to have a look. The public LspService::builder() constructor and LspServiceBuilder<S> types haven't been created yet, but the server works as expected and the custom request functionality is in place, despite not being exposed to the user yet. I had to temporarily disable $/cancelRequest support while I work on figuring out how to mesh the existing jsonrpc::pending::* module with this new request router, though. Once that's done, though, it should be ready for you to review, if you're interested!

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

Successfully merging a pull request may close this issue.

2 participants