ebkalderon / tower-lsp

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

Notification methods return Invalid request #388

Closed tekumara closed 1 year ago

tekumara commented 1 year ago

According to the spec processed notification methods should not return a response.

But I've noticed notification methods return a response with the message "Invalid request" (as per here).

eg: a textDocument/didOpen request returns the response

{"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid request"},"id":2}

This took me by surprise and I've spent awhile trying to figure out if I have indeed made an invalid request 😅

tekumara commented 1 year ago

I'm probably doing something wrong, because it's not calling my did_open method in my server... Here's the full sequence of requests and responses:

2023-03-20T11:46:23.188414Z TRACE tower_lsp::codec: <- {"jsonrpc": "2.0","method": "initialize","params": {"capabilities": {"textDocumentSync": 1}},"id": 1}
2023-03-20T11:46:23.189896Z TRACE tower_lsp::codec: -> {"jsonrpc":"2.0","result":{"capabilities":{"textDocumentSync":1},"serverInfo":{"name":"typos-lsp","version":"0.1.0"}},"id":1}
2023-03-20T11:46:23.190181Z TRACE tower_lsp::codec: <- {
                "jsonrpc": "2.0",
                "method": "textDocument/didOpen",
                "params": {
                  "textDocument": {
                    "uri": "file:///foo.rs",
                    "languageId": "rust",
                    "version": 1,
                    "text": "foobar"
                  }
                },
                "id": 2
              }
2023-03-20T11:46:23.190374Z TRACE tower_lsp::codec: -> {"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid request"},"id":2}
ebkalderon commented 1 year ago

Hi there, @tekumara! According to the JSON-RPC 2.0 spec, if an incoming message contains an id field, then is not a notification but a request, and therefore a response must be sent back.

As seen in the quoted log output above:

2023-03-20T11:46:23.190181Z TRACE tower_lsp::codec: <- {
                "jsonrpc": "2.0",
                "method": "textDocument/didOpen",
                "params": {
                  "textDocument": {
                    "uri": "file:///foo.rs",
                    "languageId": "rust",
                    "version": 1,
                    "text": "foobar"
                  }
                },
                "id": 2
              }
2023-03-20T11:46:23.190374Z TRACE tower_lsp::codec: -> {"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid request"},"id":2}

The textDocument/didOpen message is being sent with an id key with a value of 2, making this a JSON-RPC request (not a notification). However, since the textDocument/didOpen method is registered on the server as a notification and not a request, the server responds with an Invalid request message indicating that the method has been called improperly.

If you remove the "id": 2 from the above message and attempt to call the method again, the behavior should be as expected and no response will be sent back. Does this make sense?

ebkalderon commented 1 year ago

As an aside, I'd be happy to clarify what the code snippet previously linked above here actually does.

The id: Option<Id> argument determines whether a response should be emitted in reaction to some incoming message. If the argument is None, then the incoming message is a notification (that is, the incoming JSON object did not contain an "id" field) and no response should ever be sent, even if in case of a server error, missing route, invalid params, etc. However, if the incoming message is a request (the incoming JSON object does contain an "id" field), then a response must be sent, regardless of whether the RPC method in question has been registered as a request or notification on the server side.

The relevant section in the JSON-RPC 2.0 specification seems to confirm this interpretation:

A Notification is a Request object without an "id" member. A Request object that is a Notification signifies the Client's lack of interest in the corresponding Response object, and as such no Response object needs to be returned to the client. The Server MUST NOT reply to a Notification, including those that are within a batch request.

My understanding of this part of the specification is that if the client includes an "id" field, it expects a response object to be sent back, and the server is obligated to provide one. If the method being called is registered as a notification on the server, which inherently cannot provide a response, then an Invalid request error response (with the original "id") should be returned indicating that the method exists but has been called incorrectly.

ebkalderon commented 1 year ago

I thought I'd leave a TL;DR here, in addition to the above two responses. In short, the textDocument/didOpen method is indeed registered as a notification on the server side, but the incoming message shown in the trace logs contains an "id" field. That field is expected for request objects only, and textDocument/didOpen is a notification method. The server is obligated to respond to all incoming request objects with a corresponding response object (as per the spec), so it responds with an error. This is expected behavior for all JSON-RPC and LSP servers (for reference, clangd behaves exactly the same way).

Removing the unexpected "id" field from the message should fix the specific error you're seeing. Hope this helps! :+1:

tekumara commented 1 year ago

Oh I see! Thank you for walking me through that. It does indeed work when I remove the id field in the request. 🤭

ebkalderon commented 1 year ago

No worries. Glad you found the explanation helpful! The JSON-RPC 2.0 protocol (the base upon which LSP sits) is poorly specified in several key areas, IMO. I've found that some level of confusion and surprise is inevitable when interpreting the official spec, haha. To date, I've found that fuzzing the most popular JSON-RPC implementations in the wild and mimicking their behavior in tower-lsp has been just as instructive as reading the official spec, if not more so. Hope this assists your debugging efforts going forward! :smile: