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

VS Code doesn't call shutdown when exiting the standard client #2471

Closed
snjeza opened this issue May 26, 2022 · 13 comments · Fixed by #2472
Closed

VS Code doesn't call shutdown when exiting the standard client #2471

snjeza opened this issue May 26, 2022 · 13 comments · Fixed by #2472
Assignees

Comments

@snjeza
Copy link
Contributor

snjeza commented May 26, 2022

Environment
  • Operating System: any
  • JDK version: any
  • Visual Studio Code version: 1.67.2, code-insiders
  • Java extension version: 1.6.0
Steps To Reproduce
  • open a java project with VS Code
  • stop it
  • check the standard server log
Current Result

shutdown is not called

Expected Result

shutdown should be called

@rgrunber
Copy link
Member

How did this regress ?

@snjeza
Copy link
Contributor Author

snjeza commented May 26, 2022

How did this regress ?

See https://github.com/Microsoft/vscode-languageserver-node/blob/main/README.md#3170-protocol-800-json-rpc-800-client-and-800-server

cleanup of client start and stop methods. Both methods now return a promise since these methods are async. This is a breaking change since start returned a disposable before. Extensions should now implement a deactivate function in their extension main file and correctly return the stop promise from the deactivate call. As a consequence the onReady() got removed since extensions can await the start() call (even multiple times).

This change is included to vscode-languageclient 7.1.0-next.5 - https://github.com/microsoft/vscode-languageserver-node/blob/release/client/7.1.0-next.5/client/src/common/client.ts#L3335

@rgrunber
Copy link
Member

rgrunber commented May 26, 2022

We've been using 7.1.0-next.5 for some time now (83eff21#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519). I almost couldn't believe this regressed in 0.82.0, so I just tested our 1.6.0 release and it shuts everything down as expected immediately. If this change fixes things, I'm fine with merging, but am just curious at the cause of this.

@snjeza
Copy link
Contributor Author

snjeza commented May 26, 2022

so I just tested our 1.6.0 release and it shuts everything down as expected immediately.

Could you attach your standard server log?

@snjeza
Copy link
Contributor Author

snjeza commented May 26, 2022

VS Code 1.6.0

  • shutdown is not called; Java LS doesn't save the workspace

shutdown

@snjeza
Copy link
Contributor Author

snjeza commented May 26, 2022

With the PR:

  • shutdown is called; Java LS saves the workspace

shutdownpatched

@snjeza
Copy link
Contributor Author

snjeza commented May 26, 2022

I have tested VS Code Java 1.3.0, 1.4.0, 1.5.0 and 1.6.0, VS Code 1.67.2, VS Code Insiders, VS Code 1.65.0, VS Code 1.63.0 All these versions have this issue.
It is possible that the problem has been introduced by the newer VS Code.

@rgrunber
Copy link
Member

I'm able to reproduce. It's an issue. I'm not yet sure why, but if I have the MicroProfile extension installed as well, which also happens to start up, then on shutdown, the Java extension also shuts down with it. However, on its own, the Java extension remains up :\

@rgrunber
Copy link
Member

@snjeza , I just tried October 2021 (version 1.62.3) + redhat.java 1.4.0. Can you confirm that worked ?

@snjeza
Copy link
Contributor Author

snjeza commented May 27, 2022

I just tried October 2021 (version 1.62.3) + redhat.java 1.4.0. Can you confirm that worked ?

Yes, I can.

@testforstephen
Copy link
Collaborator

cleanup of client start and stop methods. Both methods now return a promise since these methods are async. This is a breaking change since start returned a disposable before. Extensions should now implement a deactivate function in their extension main file and correctly return the stop promise from the deactivate call. As a consequence the onReady() got removed since extensions can await the start() call (even multiple times).

@snjeza Nice catch! thanks. This might mitigate the issue microsoft/vscode-java-pack#954 as well.

Starting from [email protected], the BaseLanguageClient.start() has been changed to return a promise instead of Dispose, and onReady() is removed, they're breaking changes to vscode-java.

https://github.com/microsoft/vscode-languageserver-node/blob/release/client/8.0.1/client/src/common/client.ts#L933

We need to make changes to the client code in vscode-java to support the new behavior. Created an issue #2474 to track the future changes.

this.languageClient.onReady().then(() => {

@rgrunber
Copy link
Member

Just one thing. We aren't running vscode-languageclient 8.0.1. We're still on 7.1.0-next.5. So how does that change affect us now. Did the VS Code platform change something about the extension API after 1.62.3 ?

On a side note, relying on parent process watcher to kill the LS might be the cause of many corrupted workspaces (especially in bigger projects) so fixing things like this could yield a lot of benefit.

@testforstephen
Copy link
Collaborator

Just one thing. We aren't running vscode-languageclient 8.0.1. We're still on 7.1.0-next.5. So how does that change affect us now. Did the VS Code platform change something about the extension API after 1.62.3 ?

On a side note, relying on parent process watcher to kill the LS might be the cause of many corrupted workspaces (especially in bigger projects) so fixing things like this could yield a lot of benefit.

The stop method change doesn't change any LSP feature behavior, I don't think VS Code platform has updated its extension API for this. The language client is owned by each extension, and its start() and stop() is called by extension itself. If vscode-java stays at 7.1.0-next.5, we don't need to deal with start() breaking now.

I remember we had an idea to call SaveManager manually when we shutdown language server, this might help save the state in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants