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
| 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.
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.
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.
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.
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.
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.
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?
|
🆙 📅 |
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
ShutdownAsynctarget on theInProcLanguageServerto detach from the diagnostic service.Implemented the
ExitAsynctarget on theInProcLanguageServerto properly dispose ourJsonRpcobject.