Repository navigation
Disposing server before initialisation is complete results in commands not being unregistered #725
Description
If a server lists commands in executeCommandProvider.commands in its capabilities, the client registers these with VS Code and adds them to the disposables list.
However, - if you dispose before this has happened, the commands are not unregistered (since they're not registered at the time of dispose) but they are still later registered as the initialisation completes. This results in "Error: command 'foo.command' already exists" if you try to restart a server before it was fully initialised.
Repro:
Add this to testServer.ts in the server capabilities:
executeCommandProvider: {
commands: ["foo.command"],
},Add a test that disposes the server immediately then tries to recreate it:
'use strict';
import * as child_process from "child_process";
import * as path from 'path';
import * as lsclient from 'vscode-languageclient/node';
suite('Client integration', () => {
const serverModule = path.join(__dirname, './servers/testServer.js');
const documentSelector: lsclient.DocumentSelector = [{ scheme: 'lsptests', language: 'bat' }];
const middleware = {};
const clientOptions: lsclient.LanguageClientOptions = {
documentSelector,
synchronize: {},
initializationOptions: {},
middleware
};
test.only('Can dispose and re-create language server', async () => {
while (true) {
const process = child_process.spawn(`node`, [serverModule, '--stdio']);
process.stderr.on("data", (data) => console.error(data.toString()));
process.on("exit", () => console.log(`LSP Server quit!`));
console.log(`Creating server`);
const client = new lsclient.LanguageClient(
'test svr',
'Test Language Server',
() => Promise.resolve({ reader: process.stdout, writer: process.stdin }),
clientOptions,
);
console.log(`Starting server`);
const disp = client.start();
console.log(`Disposing server`);
disp.dispose();
console.log(`Waiting for server to be ready`);
await client.onReady();
await new Promise((resolve) => setTimeout(resolve, 3000));
console.log(`...`);
}
}).timeout(99999);
});
Dirk Bäumer (@dbaeumer) any suggestions on how this should be handled? I would guess that after calling dispose/stop, it should probably prevent further initialisation code from running (or at least, anything that registers disposables)?
Activity
dbaeumer commentedon Oct 22, 2021
I fixed it by ensuring that the client is ready before allowing a stop(). Everything else gets very complicated in terms of ensuring that all state can be cleaned up correctly.
Fixes microsoft#725: Disposing server before initialisation is comple…
DanTup commentedon Sep 26, 2024
Dirk Bäumer (@dbaeumer)
I'm hitting this issue again, and I'm struggling to understand what the correct way is if I need to restart and/or shutdown the language server. If I try to
stop()the server, then shortly after terminate my LSP server process, and then start a new one, I get a bunch of errors like:I tried using things like
needsStop()to ensure I'm calling things correctly, but hit other issues (see #1559). I need to be able to callkill()on my server process a short time after callingstop()even if the server is unresponsive to ensure we do not leave orphaned processes.Could you describe what the correct process is for stopping an LSP client (in any state), and then ensuring the underlying server process can be terminated without showing errors, and ensuring that all commands have been unregistered?
I think what I'd really like is a function we can call at any time to ask for shutdown, which immediately unregisters all VS Code commands/providers, puts the client into a silent mode (do not show any errors caused by shutting down), and then attempt to gracefully shut the server down with a timeout. This would allow me to call
kill()on the process after calling this method, and not have to worry about what state the server is (or if multiple code paths try to shut down the server around the same time).dbaeumer commentedon Sep 30, 2024
How do you start the server and which communication mechanism do you use to talk to it?
DanTup commentedon Sep 30, 2024
I spawn the process and use
StreamMessageReader/StreamMessageWriterso that we can a) log the raw traffic for debugging and b) attach a timestamp to all outbound messages (so we can measure latency between this and when the server begins processing the message):The code to start the client is like this
dbaeumer commentedon Oct 1, 2024
Lets move the discussion into #1559 to have everything in one place since I guess these issues are related.
One problem might be that the server process is not finished since the client doesn't know how to kill it. One solution might be to pass in a terminate callback into the restart method.