denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.44k stars 5.37k forks source link

LSP not responding when editor quits and sends shutdown request #20700

Open cngarrison opened 1 year ago

cngarrison commented 1 year ago

When quitting my EDITOR (BBEdit), it hangs waiting for deno lsp.

I have reported the issue to BareBones. They advised that it's possibly due to LSP server not responding correctly to the application's request to shut itself down.

Does deno lsp respond to the shutdown request?

Is there a config setting for handling the shutdown request?

nayeemrmn commented 1 year ago

The shutdown handler we have just tries to acquire the global mutex it has (ensuring in-flight operations finish) and then responds with Ok(()). If it's really hanging, maybe it's because a previous request going either way is on-going. I don't think this is been a problem with other editors though. Unfortunately I can't investigate very far since BBEdit's source isn't public.

@cngarrison Does this happen with a bare minimum reproduction? E.g. open an empty ts file in a fresh directory, plus whichever method you're using to enable the Deno LSP, and try to exit.

cngarrison commented 1 year ago

@nayeemrmn Thanks for that feedback.

I launched BBEdit with deno LSP server enabled and no documents open. I then quit BBEdit and it quit cleanly.

I launched BBEdit, opened a new project directory (file browser), created an (empty) index.ts file, then quit BBEdit. The hang returned.

The team at BareBones is very good, and I have flagged this issue with them. Please advise if there is any further testing I can help with.

nayeemrmn commented 1 year ago

When quitting my EDITOR (BBEdit), it hangs waiting for deno lsp.

I have reported the issue to BareBones. They advised that it's possibly due to LSP server not responding correctly to the application's request to shut itself down.

@sigmaSd Do you know of any quirks that might relate to this?

siegel commented 1 year ago

For background (and in case it helps focus the investigation):

When stopping a language server (at application quit or for certain configuration changes), BBEdit sends a shutdown request to the server and waits for a response.

After receiving a response to shutdown, BBEdit then sends the server an exit notification.

Then, BBEdit waits for the server process to exit. This is done because some language servers actually do nothing at shutdown time, and instead do all of their cleanup (index database commits, and the like) when they receive an exit, and in some cases this can take a number of seconds. (An example of this is found in clangd .)

In the case of deno, it responds normally (and rapidly) to shutdown, but after receiving the exit notification, the deno process never terminates.

If deno has any work to do when shutting the server down (such as committing an index database, or other housekeeping), the shutdown handler is the time to do this; and the exit notification handler simply needs to terminate the server process.

Hope this helps. :-)

sigmaSd commented 1 year ago

Its never been a problem on nvim/helix, so I don't have other informations regarding this.

nayeemrmn commented 1 year ago

@siegel Is there a chance BBEdit isn't closing the LSP process's stdin?

siegel commented 1 year ago

@siegel Is there a chance BBEdit isn't closing the LSP process's stdin?

Probably. :-) BBEdit does not explicitly close the pipes until after the server process has exited as requested.

Is there something in the LSP specification that states that the client is expected to close stdin after sending the exit notification?

nayeemrmn commented 1 year ago

Is there something in the LSP specification that states that the client is expected to close stdin after sending the exit notification?

Nope. Based on the wording at https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit we should exit without needing that. But given that this issue occurs specifically between Deno and BBEdit, maybe both actions should be taken

@ebkalderon Could Server::serve() be changed to break its loop on exit notifications without waiting for stdin to close, so our program can finish up as normal? Or is there something we can do currently?

ebkalderon commented 1 year ago

@nayeemrmn Certainly, this could (and should) definitely be changed. In theory, Server::serve() should already gracefully shut down when it receives an exit message, regardless of the state of stdin, but if it doesn't, then there must be a bug in the logic somewhere. Please feel free to open a bug report on the tower-lsp repo to investigate further. Thanks for bringing this to my attention, folks!

siegel commented 1 year ago

Please feel free to open a bug report on the tower-lsp repo to investigate further. Thanks for bringing this to my attention, folks!

Done! Your #399 is it. And I appreciate you taking an interest in looking into it. :-)

sgwilym commented 11 months ago

I'm experiencing the same issue in Nova with the built-in LanguageClient. Manually restarting the LSP client often causes the server to hang, and after a time a stream of The request was cancelled. messages come through.