Dispose orphaned progress notifications behind when using progress reporting in language servers

[joyceerhl]

Environment data

• VS Code version: 1.59.0-insider
• Extension version (available under the Extensions sidebar): v2021.8.1046297456-dev
• Pylance version: v2021.7.5-pre.1
• OS and version: Windows 10
• Python version (& distribution if applicable, e.g. Anaconda): Python 3.9.6
• Type of virtual environment used (N/A | venv | virtualenv | conda | ...): N/A
• Relevant/affected Python packages and their versions: N/A
• Relevant/affected Python-related VS Code extensions and their versions: N/A
• Value of the python.languageServer setting: XXX

[NOTE: If you suspect that your issue is related to the Microsoft Python Language Server (python.languageServer: 'Microsoft'), please download our new language server Pylance from the VS Code marketplace to see if that fixes your issue]

Expected behaviour

Language server shouldn't crash. If it does progress notifications should be disposed

Actual behaviour

Steps to reproduce:

[NOTE: Self-contained, minimal reproducing code samples are extremely helpful and will expedite addressing your issue]

1. Open a Python file

Logs

pylance.log
python.log

[karthiknadig]

@joyceerhl Can you look for the stack from one of the extension host logs that might have more detail on where the crash occurred?

/cc @jakebailey is there a any additional logging you need here.

[jkryanchou]

+1

[jakebailey]

The log message and the python language server logs indicate that this is not Pylance, but MPLS (crashing with a stack overflow.

[karrtikr]

@jkryanchou We have recently released a new and more performant language server called Pylance which likely solves these issues you are reporting.  You can download Pylance from the marketplace and install it alongside the core Python extension. Because our team's development efforts have shifted to support the new language server, we'd encourage you to try it out. If you try out Pylance and this issue persists, you can open an issue on the pylance-release repo.

[joyceerhl]

I have "python.languageServer": "Pylance", and the Pylance extension is installed. I'm still getting the Python Tools server crashing.

[jakebailey]

The language server must be getting set somewhere else; does the UI also say that its Pylance or Default?

[joyceerhl]

Ah thanks, Jupyter repo workspace settings still say Microsoft. I'd expect the in progress notification that is presumably put up by the Python extension to be disposed if the server crashes though.

[jakebailey]

I don't know why those are even showing up there; the API that runs those puts the message at the bottom bar of the editor, not as a real notification. Unless opening it manually brings that up?

[joyceerhl]

Yeah so the notification sticks around in the status bar and if you click on it it shows that there's five of those progress notifications going that can't be cancelled.

[jakebailey]

Ah, I see.

The LS typically controls the start and end, but I guess the client should register something that stops this if the LS exits or something.

But, I'd also argue that if the LS is crashing, there's a worse problem than some stale notifications; if it crashes once, it's likely going to crash again when it gets restarted.

[karrtikr]

From the extension side all we can do is dispose of all notifications related to Microsoft language server. As we'll be getting rid of it once we remove old LS I don't see much point of keeping this issue open.

[jakebailey]

This same API is used for pylance for its progress messages (though it uses them less), so it's technically still a problem.

[karrtikr]

Can you point to the places where Pylance uses progress notifications in our repo? I'm unable to repro it in Pylance.

[jakebailey]

https://github.com/microsoft/vscode-python/blob/3698950c97982f31bb9dbfc19c4cd8308acda284/src/client/activation/progress.ts is the implementation, shared between all LSs.

Pylance only displays a progress message if you have the diagnostic mode set to "workspace" (where it takes more time to typecheck the entire workspace), but unless you're in a large project, you're not going to see it as Pylance is too quick to have the message up for a long time (unlike MPLS).

[jakebailey]

But, I'm actually not sure if we use this in Pylance anymore, now that I think about it. Progress reporting got added to the LSP itself, so we may be using that. I'll check.

[jakebailey]

No, we're still using this API in Pylance, so it could feasibly affect us. I haven't seen anyone hit it, though.

[karrtikr]

Got it, I guess we should be calling this.dispose() at line 39

vscode-python/src/client/activation/progress.ts

Lines 37 to 43 in db8e1e2

	this.languageClient.onNotification('python/endProgress', (_) => {
	if (this.progressDeferred) {
	this.progressDeferred.resolve();
	this.progressDeferred = undefined;
	this.progress = undefined;
	}
	});

Although the API dispose() is also exposed so maybe the consumers should be responsible of calling it when done?

[druuuu]

Hi I'm interested in sending a PR for this

[luabud]

@druuuu great! I assigned you to the issue 😊

[Kavvyashre1395]

Please assign this bug.

[luabud]

H @Kavvyashre1395, since it's been one month since OSD I'm removing you as an assignee. You can still feel free to submit a PR to this issue if you'd like, but we're going to leave it open for other contributors too.