-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add support for language server exit & shutdown. #44900
Conversation
- The language server platform is properly implementing solution close understanding for local language servers and I found that when testing their bits Roslyn would explode gloriously. Turns out shutdown and exit support just wasn't fully enabled yet. - When the language server platform reboots language servers it re-invokes Activate and because of that Roslyn explodes because the language client has already been activated and there was previously a contract throws check on already being initialized. - Implemented the `ShutdownAsync` target on the `InProcLanguageServer` to detach from the diagnostic service. - Implemented the `ExitAsync` target on the `InProcLanguageServer` to properly dispose our `JsonRpc` object.
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/LanguageClient/AbstractLanguageServerClient.cs
Show resolved
Hide resolved
🆙 📅 |
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Outdated
Show resolved
Hide resolved
@@ -68,7 +68,7 @@ internal abstract class AbstractLanguageServerClient : ILanguageClient | |||
|
|||
public Task<Connection> ActivateAsync(CancellationToken token) | |||
{ | |||
Contract.ThrowIfFalse(_languageServer == null, "This language server has already been initialized"); | |||
Contract.ThrowIfTrue(_languageServer?.Running == true, "The language server has not yet shutdown."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? Given the Activate() creates a new StreamJsonRpc and then the Shutdown/Exit are called on that specific instance, is there any reason we can't simply have two instances activated at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really ever want two instances of the same language server activated - the contract statement has caught bugs in the past around us / lsp client accidentally activating multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is this guarding our code (that we can't support this if it was ever needed) or is it just catching bugs in other components? Is there an API doc that says we can't expect multiple activations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Activation of the ILanguageClient triggers https://microsoft.github.io/language-server-protocol/specification#initialize which is spec'd to only allow initialize once per server.
Potentially for the inproclanguageserver we could support it, but for a connection to the OOP server, I don't know if we want to create a new connection every time (which reminds me, filed #44998 to support shutdown there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So "only once per server" is different than "there is only one server". If you squint and consider ActivateAsync to be "CreateANewServerAsync", is there any reason that's disallowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is (was?) a contract that activateasync is called once per ILanguageClient instance (which is why the contract was originally written), though I couldn't find the document on it
@tinaschrepfer that is still the case correct?
🆙 📅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good with me!
The language server platform is properly implementing solution close understanding for local language servers and I found that when testing their bits Roslyn would explode gloriously. Turns out shutdown and exit support just wasn't fully enabled yet.
When the language server platform reboots language servers it re-invokes Activate and because of that Roslyn explodes because the language client has already been activated and there was previously a contract throws check on already being initialized.
Implemented the
ShutdownAsync
target on theInProcLanguageServer
to detach from the diagnostic service.Implemented the
ExitAsync
target on theInProcLanguageServer
to properly dispose ourJsonRpc
object.