attempt to fix a race where new LS starts before previous LS stopped.

[heejaechang]

From crash report, we found a possible race situation where new Pylance is started before old Pylance is disposed causing duplicate commands to be registered and error out by LSP client.

this is an attempt to fix the race by making sure LSP server stop is awaited before calling start eliminating all weird cases and we no longer assume that languge client will be reused (start/stop and start again).

[heejaechang]

by the way, does anyone know when settings.json is changed, which code path in core extension call onDidChangeConfiguration on us?

[kimadeline]

which code path in core extension call onDidChangeConfiguration on us?

What do you mean? Who fires the onDidChangeConfiguration event?

[heejaechang]

What do you mean? Who fires the onDidChangeConfiguration event?

figured out, it was LSP client itself raising the event. by the way, found issue on LSP itself as well
microsoft/vscode-languageserver-node#929

[heejaechang]

alright, it looks like whatever fix I made here in proxy won't fix the race. it turns out unlike before (< 2022.4.x), now, when interpreter is changed, completely new language server is created (sorry, I just found out once I couldn't figure out how the state change it is having happened). so, race can't be fixed in proxy, but must be fixed in
https://github.com/microsoft/vscode-python/blob/main/src/client/languageServer/watcher.ts#L137 where it stops and create new LS. (and probably other places, I saw multiple code paths that end up restart server)

it looks like the change happened in this PR (#18884)

any objection on just adding stop: Promise or make dispose async on proxy and all callers to make it simple and remove any chance of race?

tagging @kimadeline @karrtikr

[kimadeline]

any objection on just adding stop: Promise or make dispose async on proxy and all callers to make it simple and remove any chance of race?

I vote for the first option, not so sure about making dispose async for everything.

[heejaechang]

do you guys use cancellation? I don't see code path involved here use any cancellation. for example, if someone flip thorough different version of interpreters, in current form, every selection must start server and stop them one by one. rather than all the intermediate one gets cancelled if server is not started yet and only last selection start server?

[heejaechang]

@kimadeline can you take a look?

[kimadeline]

Nope, we don't use cancellation, although it would make sense to use it. Do you think that would be a big change to the current code?

[kimadeline]

Also, there will be merge conflicts depending on which PR gets merged in first between this one and #19087.