lsp-server: handle_shutdown crashing on response

[fda-odoo]

Hello,

We are using your crate lsp-server in our project, and we face an issue when exiting the server.
There is a method that handle the shutdown Request from the client:

rust-analyzer/lib/lsp-server/src/lib.rs

Line 348 in 3c2aca1

pub fn handle_shutdown(&self, req: &Request) -> Result<bool, ProtocolError> {

This method receive the shutdown request, then send the response and wait for the exit notification, sending an error if another message arrives.
But according to the lsp specification (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#shutdown) , it is normal in my understanding that the client do not send any new Request after sending the shutdown request, but he could still send Responses to previous requests. ("If a server receives requests after a shutdown request those requests should error with InvalidRequest")
If that's the case, handle_shutdown is erroring, and we lose the responses we are still waiting.
We use the vscode client to develop our server, so I guess that receive Responses after the shutdown request is valid regarding the specifications.
We are thinking about modifying the line https://github.com/rust-lang/rust-analyzer/blob/3c2aca1e5e9fbabb4e05fc4baa62e807aadc476a/lib/lsp-server/src/lib.rs#L356C13-L356C25 , but we wanted to share this feedback with you and know what you think about that

Have a nice day

[ChayimFriedman2]

Why does it matter if we ignore the responses? We're shutting down anyway. Is the error the problem?

[fda-odoo]

Yes, it means we do not handle the exit notification and leave this method with an error, instead of really handle the exit notification. So the method is not really doing what it it designed to do, and send a ProtocolError where there is not.
In our case it it created error logs when closing the program, and ok it's not that bad, as we are closing anyway, but it prevents us to get responses to requests before closing, and log for nothing.
We fixed it in our code by not using handle_shutdown, but I wanted to share it with you

[Veykril]

So to clarify, the issue is that the receiver there might receive a Response that the server wants to send instead, causing us to take the wrong match arm and hence not actually handle the exit notification later on? That does sound like a problem we should fix. I'd imagine we should just loop until we get the exit notification, send out all responses the server wants to send and send errors for new requests, is that correct? (looks like we aren't even using that function ourselves)

[fda-odoo]

Indeed, that's what we are doing on our side.
Let's imagine that the server send a request to the client, that requiert a Response (see image below)
But in the meantime, the client initiates a shutdown. The client, waiting for the answer of the server for the shutdown, see that he can send a Response to the server about the first request (From what I see and what I got in vscode, nothing prevent client from sending Responses after sending a shutdown request).
Then the server receives the response of its request but was only waiting for exit notification, so raise a ProtocolError.

Indeed an easy fix would be to loop and discard responses, but it would prevent the server from effectively react to these responses, but that issue depends on the needs of the server. In our case we integrated the shutdown behaviour to our main loop because we want to handle Responses even in shutdown process.

From my point of view, I guess that "handle_shutdown" is more an "example" on how to handle shutdown and maybe shouldn't be used in production, but then it would nice that it reflects this behaviour

[Veykril]

Ah right, server to client requests exist as well (forgot about that since rust-analyzer doesn't make use of that). Yes I think that method only really exists as an example (being used in our one example as well). I believe it might be best to remove it (or well inline it into our example, and add a comment) as proper handling seems to be project specific.