client.needsStop() and client.stop() disagree on whether it's valid to call stop()

[DanTup]

I'm trying to fix some issues with the LSP client showing errors when I try to dispose it. One issue is that I'm getting errors like this:

rejected promise not handled within 1 second: Error: Client is not running and can't be stopped. It's current state is: starting
extensionHostProcess.js:155
stack trace: Error: Client is not running and can't be stopped. It's current state is: starting
	at LanguageClient.shutdown (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:38362:10)
	at LanguageClient.stop (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:38332:15)
	at LanguageClient.stop (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:47462:22)
	at Object.dispose (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:4810:66)
	at disposeAll (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:29122:20)
	at LspAnalyzer.dispose (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:25645:32)
	at d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:14511:36
	at tryCleanup (d:\Dev\Dart-Code\Dart-Code\out\dist\extension.js:14544:15)

The problem is that calling stop() is invalid when the state is stopping, however my code looks like this:

if (this.client.needsStop())
	await this.client.stop();

The implementation of needsStop is:

vscode-languageserver-node/client/src/common/client.ts

Lines 1228 to 1230 in f58f4df

	public needsStop(): boolean {
	return this.$state === ClientState.Starting || this.$state === ClientState.Running;
	}

So needsStop says that stop is required if the state is starting, but calling stop() throws. It's not clear to me what the correct way of shutting down the server cleanly is.

[dbaeumer]

How do you start the server and which transport are you using? Looking through the code I think we need to allow clients to pass in a terminate function into the client#restart() method in case it is not a process started by the client itself.

[dbaeumer]

See also #725 (comment)

[DanTup]

I think we need to allow clients to pass in a terminate function into the client#restart() method in case it is not a process started by the client itself.

I didn't realise there was a restart method, but I think this applies to just stop() too. For some cases where I restart the server, things like the active Dart SDK might be changing so I basically restart the entire extension (basically call deactivate() and then activate() internally). I think it should be possible to also do this cleanly (eg. not show the user errors if the server didn't shut down cleanly, and all registered commands must be unregistered so the new server can register them).

[dbaeumer]

Side note: have you checked that calling deactivate() and activate() on yourself is something that is supported from a VS Code extension side.

[DanTup]

Side note: have you checked that calling deactivate() and activate() on yourself is something that is supported from a VS Code extension side.

They're just functions, so I'm not sure why they wouldn't (I just call them directly). I implemented this many years ago and it hasn't had any issues (except some edge cases for LSP noted here and #725).. It was the simplest way to ensure when something fundamental (like the SDK path) changes, we dispose everything old and create new versions without just implementing our own parallel version of disposing and recreating everything.