[juliasilge]

Fixes #1209

Problem

The Outline view can show more than one "Quarto" group for one document. We have had some reports of this in Positron too here and there, but without a firm repro. Seems like this problem can occur when settings change quickly, for example while VS Code starts.

The LSP registers its document symbol provider again each time the configuration changes. Each update disposes the old registration and then waits for a new one. If two updates overlap, both updates dispose the same old registration. Then the second assignment overwrites the first new registration, so the server cannot dispose it later. Each leaked registration adds one more "Quarto" group to the Outline.

Changes

• The new documentSymbolRegistrar in apps/lsp/src/symbol-registration.ts puts updates in a queue, so only one update runs at a time. As a result, at most one registration is active, and the last value of showCodeCellsInOutline sets the final state.
• If the value of showCodeCellsInOutline did not change, the registrar does not register again. The client sends configuration and theme notifications for any setting change, so this skip also stops Outline refreshes that are not necessary.
• If a registration fails, the registrar logs the error and the queue continues. The next update tries again. Before this change, if the first registration failed during startup, mdLsReady did not resolve, and symbol and folding requests did not complete.
• The server now attaches the configuration listener before the first registration. As a result, a setting change during startup goes into the queue and is not lost.
• apps/lsp now has unit tests for overlapping updates, fast toggles, the skip, and recovery after a failed registration. The overlap and toggle tests fail against the previous code. npm run test-packages now includes quarto-lsp.

How to test

1. Open a .qmd file that has headings and code cells.
2. Show the Outline view.
3. Run Developer: Reload Window several times. Make sure that the Outline shows one "Quarto" group each time.
4. Run Quarto: Toggle Code Cells in Outline several times quickly. Make sure that the Outline shows one group, and that the code cells match the final setting.
5. Change the color theme. Make sure that the Outline keeps its expanded and collapsed state.

[vezwork]

Requested some changes to improve readability/reasonability.

This seems like an important fix, I wonder if we shouldn't be doing something like this in more places?

[vezwork]

Suggested change

	let queue: Promise<void> = Promise.resolve();
	let registration: Disposable | undefined;
	let registeredFor: boolean | undefined;
	return {
	update(showCodeCellsInOutline: boolean) {
	queue = queue.then(async () => {
	if (registration && registeredFor === showCodeCellsInOutline) {
	return;
	}
	registration?.dispose();
	registration = undefined;
	registration = await register(showCodeCellsInOutline);
	registeredFor = showCodeCellsInOutline;
	}).catch(onError);
	return queue;
	}
	};
	let previous?: {registration: Promise<Disposable>, showCodeCellsInOutline: boolean}
	return {
	async update(showCodeCellsInOutline: boolean) {
	try {
	const disposableReg = await previous?.registration
	if (disposableReg !== undefined) {
	if (previous.showCodeCellsInOutline === showCodeCellsInOutline) {
	return disposableReg
	} else {
	disposableReg.dispose()
	}
	}
	previous = {
	registration: register(showCodeCellsInOutline),
	showCodeCellsInOutline
	}
	} catch(e) { onError(e) }
	return await previous.registration
	}
	};

Not sure this code is quite right (should be close tho), I quickly hand wrote it and didn't test it. But I prefer something like this. Endlessly chaining promises and three separate mutable variables is hard to reason about.

[vezwork]

Oh I would need to take previous.registeredFor into account, I missed that. Give me a sec I'll update my suggestion.

[vezwork]

ok updated. It can probably be slightly improved still cuz I don't think disposableReg will ever be undefined if previous isn't also?

[juliasilge]

Thank you for the suggestion! I agree that one state object is much easier to read than three variables and a queue. I changed the registrar to keep one current promise. This promise holds the registration and its showCodeCellsInOutline value together, like your previous object.

I could not use your version exactly, though, because of the order of operations. Each call awaits previous.registration before it replaces previous. When two calls overlap, both get the same registration, both dispose it, and both register a new one. The second assignment then overwrites the first new registration, which is the leak from #1209. To prevent this, each update now replaces current synchronously, before any await, and then each update always waits for the update before it.

You were correct that disposableReg is only undefined when previous is also undefined! With a single Registration | undefined, that extra check is not necessary.

[vezwork]

Nice. Approved with a nitpick readability suggestion

[vezwork]

nitpick: pull this immediately invoked function out of documentSymbolRegistrar, give it a name, and pass it previous.

[vezwork]

Also, do you suspect this problem pattern of overlapping registrations could be happening elsewhere where registrations happen in response to config changes @juliasilge? Or would it just be isolated to this for some reason I don't see?

[juliasilge]

I am pretty sure that this problem is isolated, @vezwork. The leak needs two conditions together:

1. The registration is async, so another update can start between the dispose() and the assignment of the new registration.
2. The registration runs again on each config change, so updates can overlap.

The symbol provider is the only place with both. I looked at each config-change handler in apps/lsp and apps/vscode:

• The other LSP dynamic registrations (the diagnostics pull source, the file watchers, and the config notification) are async, but they run one time at startup.
• The extension-side registrations that run on config change (the Positron code lens, the host cell providers, and the embedded diagnostics manager) are synchronous. vscode.languages.register*Provider returns its Disposable immediately, so nothing can interleave. These handlers also check for an existing registration before they add one.

A sort of related item, but not a leak is that ConfigManager.update() is async, so two fast config changes can resolve out of order in theory. I don't think that this occurs in practice, and the worst result is old settings for a short time.

[vezwork]

TY nice fix!