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

Exiting during LSP initialization fixed #5850

Closed
wants to merge 1 commit into from

Conversation

misiasty3
Copy link
Contributor

Quick fix for issue #5732, by sending exit notification to language servers that are not yet initialized.

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 6, 2023
@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 6, 2023

Thanks for contributing!
This change is not the right fix for multiple reasons. The instructions in #5732 were not correct. I didn't have a closer look at that yet, sorry for the confusion.

In the send function in transport.rs we drop any notifications and delay any shutdown requests as long as the server is not initialized so this new exit notification will never be send. In #5732 you can see the client waiting for initialization as indicated by the Language server not initialized, delaying request log message and hence the shutdown request is never sent in time (and neither the subsequent exit).

The right fix is to always send exit notifications and shutdown requests when the server is not initialized. Simply add a special conditions in transports.rs:371 something like && !is_exit_or_shutdown() (the latter being a function that needs to be implemented).

The reason we should shutdown too is that we are actually kind of racing the server initalization here so the server might finish initialization before we know about.
To still properly shutdown the server in that case we must always send shutdown.
If the server is not initialized this is not problem as the server will simply drop the request in that case. From the LSP standard:

For a request the response should be an error with code: -32002. The message can be picked by the server

With that change to the send function the only other required function is to change the shutdown_and_exit so that

pub async fn shutdown_and_exit(&self) -> Result<()> {
    if let Err(e) = self.shutdown().await {
    // if the server is still initalizing it will return -32002 in that case it's save to send exit() directly
	if self.initalized && matches!(e, Err(Error::Rpc(jsonrpc::Error{code: jsonrpc::ErrorCode::ServerError(-32002), ..) {
             return Err(e);
        }
    }
    self.shutdown().await?;
}

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 6, 2023
@pascalkuthe
Copy link
Member

Ah actually reading further in the spec I am not quite sure if this is the correct solution afterall:

Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server

That means we could send an exit notification before even asking the server to initialize but that seems very nieche and is not what #5732 is about (the initalize request gets send basically instantly). The initialization itself takes too long (so after initialize is send). We are not allowed to send anything in that case. I think in that case just timing out is actually the correct thing to do.

Nothing we can really do about this I believe. So helix is behaving correctly here, might be interesting to look into what other editors in this case tough.

@pascalkuthe pascalkuthe removed the E-easy Call for participation: Experience needed to fix: Easy / not much label Feb 6, 2023
@pascalkuthe
Copy link
Member

See microsoft/language-server-protocol#246 pretty sure this is a current limitation of the LSP spec and there is nothing we can do about this.

@the-mikedavis
Copy link
Member

This is fixed in #7449: now we follow neovim's behavior and kill the language server when shutting down during initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants