Disposing server before initialisation is complete results in commands not being unregistered

[DanTup]

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);
});

@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)?

[dbaeumer]

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.

[DanTup]

@dbaeumer

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.

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 call kill() on my server process a short time after calling stop() 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]

How do you start the server and which communication mechanism do you use to talk to it?

[DanTup]

I spawn the process and use StreamMessageReader/StreamMessageWriter so 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

const client = new LanguageClient(
	"dartAnalysisLSP",
	"Dart Analysis Server",
	async () => {
		const streamInfo = await this.spawnServer(logger, sdks, dartCapabilities); // This spawns the server and logs the streams
		const jsonEncoder = ls.RAL().applicationJson.encoder;

		return {
			detached: streamInfo.detached,
			reader: new StreamMessageReader(streamInfo.reader),
			writer: new StreamMessageWriter(streamInfo.writer, {
				contentTypeEncoder: {
					encode: (msg, options) => {
						(msg as any).clientRequestTime = Date.now(); // This adds the timestamp
						return jsonEncoder.encode(msg, options);
					},
					name: "withTiming",
				},
			}),
		};
	},
	clientOptions,
);

// And shortly after:
client.start()

[dbaeumer]

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.