Fix AsyncDataTree concurrent refresh/expand calls (#80300)

* failing test for #80098

* AsyncDataTree: watch out for concurrent refresh and expand calls

fixes #80098

* remove explorer workaround

* cleanup rogue import

src/vs/base/browser/ui/tree/asyncDataTree.ts

@@ -24,7 +24,7 @@ interface IAsyncDataTreeNode<TInput, T> {
 	readonly parent: IAsyncDataTreeNode<TInput, T> | null;
 	readonly children: IAsyncDataTreeNode<TInput, T>[];
 	readonly id?: string | null;
-	loading: boolean;
+	refreshPromise: Promise<void> | undefined;
 	hasChildren: boolean;
 	stale: boolean;
 	slow: boolean;
@@ -41,7 +41,7 @@ function createAsyncDataTreeNode<TInput, T>(props: IAsyncDataTreeNodeRequiredPro
 	return {
 		...props,
 		children: [],
-		loading: false,
+		refreshPromise: undefined,
 		stale: true,
 		slow: false,
 		collapsedByDefault: undefined
@@ -468,8 +468,8 @@ export class AsyncDataTree<TInput, T, TFilterData = void> implements IDisposable
 			throw new TreeError(this.user, 'Tree input not set');
 		}
 
-		if (this.root.loading) {
-			await this.subTreeRefreshPromises.get(this.root)!;
+		if (this.root.refreshPromise) {
+			await this.root.refreshPromise;
 			await Event.toPromise(this._onDidRender.event);
 		}
 
@@ -519,21 +519,26 @@ export class AsyncDataTree<TInput, T, TFilterData = void> implements IDisposable
 			throw new TreeError(this.user, 'Tree input not set');
 		}
 
-		if (this.root.loading) {
-			await this.subTreeRefreshPromises.get(this.root)!;
+		if (this.root.refreshPromise) {
+			await this.root.refreshPromise;
 			await Event.toPromise(this._onDidRender.event);
 		}
 
 		const node = this.getDataNode(element);
 
-		if (node !== this.root && !node.loading && !this.tree.isCollapsed(node)) {
+		if (node.refreshPromise) {
+			await this.root.refreshPromise;
+			await Event.toPromise(this._onDidRender.event);
+		}
+
+		if (node !== this.root && !node.refreshPromise && !this.tree.isCollapsed(node)) {
 			return false;
 		}
 
 		const result = this.tree.expand(node === this.root ? null : node, recursive);
 
-		if (node.loading) {
-			await this.subTreeRefreshPromises.get(node)!;
+		if (node.refreshPromise) {
+			await this.root.refreshPromise;
 			await Event.toPromise(this._onDidRender.event);
 		}
 
@@ -677,18 +682,18 @@ export class AsyncDataTree<TInput, T, TFilterData = void> implements IDisposable
 			return result;
 		}
 
-		result = this.doRefreshSubTree(node, recursive, viewStateContext);
-		this.subTreeRefreshPromises.set(node, result);
-
-		try {
-			await result;
-		} finally {
-			this.subTreeRefreshPromises.delete(node);
-		}
+		return this.doRefreshSubTree(node, recursive, viewStateContext);
 	}
 
 	private async doRefreshSubTree(node: IAsyncDataTreeNode<TInput, T>, recursive: boolean, viewStateContext?: IAsyncDataTreeViewStateContext<TInput, T>): Promise<void> {
-		node.loading = true;
+		let done: () => void;
+		node.refreshPromise = new Promise(c => done = c);
+		this.subTreeRefreshPromises.set(node, node.refreshPromise);
+
+		node.refreshPromise.finally(() => {
+			node.refreshPromise = undefined;
+			this.subTreeRefreshPromises.delete(node);
+		});
 
 		try {
 			const childrenToRefresh = await this.doRefreshNode(node, recursive, viewStateContext);
@@ -696,7 +701,7 @@ export class AsyncDataTree<TInput, T, TFilterData = void> implements IDisposable
 
 			await Promise.all(childrenToRefresh.map(child => this.doRefreshSubTree(child, recursive, viewStateContext)));
 		} finally {
-			node.loading = false;
+			done!();
 		}
 	}
 

src/vs/base/test/browser/ui/tree/asyncDataTree.test.ts

@@ -8,6 +8,7 @@ import { ITreeNode, ITreeRenderer, IAsyncDataSource } from 'vs/base/browser/ui/t
 import { AsyncDataTree } from 'vs/base/browser/ui/tree/asyncDataTree';
 import { IListVirtualDelegate, IIdentityProvider } from 'vs/base/browser/ui/list/list';
 import { hasClass } from 'vs/base/browser/dom';
+import { timeout } from 'vs/base/common/async';
 
 interface Element {
 	id: string;
@@ -379,4 +380,145 @@ suite('AsyncDataTree', function () {
 		assert(!tree.getNode(_('a')).collapsed);
 		assert.deepStrictEqual(getChildrenCalls, ['root', 'a']);
 	});
+
+	test('issue #80098 - concurrent refresh and expand', async () => {
+		const container = document.createElement('div');
+		container.style.width = '200px';
+		container.style.height = '200px';
+
+		const delegate = new class implements IListVirtualDelegate<Element> {
+			getHeight() { return 20; }
+			getTemplateId(element: Element): string { return 'default'; }
+		};
+
+		const renderer = new class implements ITreeRenderer<Element, void, HTMLElement> {
+			readonly templateId = 'default';
+			renderTemplate(container: HTMLElement): HTMLElement {
+				return container;
+			}
+			renderElement(element: ITreeNode<Element, void>, index: number, templateData: HTMLElement): void {
+				templateData.textContent = element.element.id;
+			}
+			disposeTemplate(templateData: HTMLElement): void {
+				// noop
+			}
+		};
+
+		const calls: Function[] = [];
+		const dataSource = new class implements IAsyncDataSource<Element, Element> {
+			hasChildren(element: Element): boolean {
+				return !!element.children && element.children.length > 0;
+			}
+			getChildren(element: Element): Promise<Element[]> {
+				return new Promise(c => calls.push(() => c(element.children)));
+			}
+		};
+
+		const identityProvider = new class implements IIdentityProvider<Element> {
+			getId(element: Element) {
+				return element.id;
+			}
+		};
+
+		const root: Element = {
+			id: 'root',
+			children: [{
+				id: 'a', children: [{
+					id: 'aa'
+				}]
+			}]
+		};
+
+		const _: (id: string) => Element = find.bind(null, root.children);
+
+		const tree = new AsyncDataTree<Element, Element>('test', container, delegate, [renderer], dataSource, { identityProvider });
+		tree.layout(200);
+
+		const pSetInput = tree.setInput(root);
+		calls.pop()!(); // resolve getChildren(root)
+		await pSetInput;
+
+		const pUpdateChildrenA = tree.updateChildren(_('a'));
+		const pExpandA = tree.expand(_('a'));
+		assert.equal(calls.length, 1, 'expand(a) still hasn\'t called getChildren(a)');
+
+		calls.pop()!();
+		assert.equal(calls.length, 0, 'no pending getChildren calls');
+
+		await pUpdateChildrenA;
+		assert.equal(calls.length, 0, 'expand(a) should not have forced a second refresh');
+
+		const result = await pExpandA;
+		assert.equal(result, true, 'expand(a) should be done');
+	});
+
+	test('issue #80098 - first expand should call getChildren', async () => {
+		const container = document.createElement('div');
+		container.style.width = '200px';
+		container.style.height = '200px';
+
+		const delegate = new class implements IListVirtualDelegate<Element> {
+			getHeight() { return 20; }
+			getTemplateId(element: Element): string { return 'default'; }
+		};
+
+		const renderer = new class implements ITreeRenderer<Element, void, HTMLElement> {
+			readonly templateId = 'default';
+			renderTemplate(container: HTMLElement): HTMLElement {
+				return container;
+			}
+			renderElement(element: ITreeNode<Element, void>, index: number, templateData: HTMLElement): void {
+				templateData.textContent = element.element.id;
+			}
+			disposeTemplate(templateData: HTMLElement): void {
+				// noop
+			}
+		};
+
+		const calls: Function[] = [];
+		const dataSource = new class implements IAsyncDataSource<Element, Element> {
+			hasChildren(element: Element): boolean {
+				return !!element.children && element.children.length > 0;
+			}
+			getChildren(element: Element): Promise<Element[]> {
+				return new Promise(c => calls.push(() => c(element.children)));
+			}
+		};
+
+		const identityProvider = new class implements IIdentityProvider<Element> {
+			getId(element: Element) {
+				return element.id;
+			}
+		};
+
+		const root: Element = {
+			id: 'root',
+			children: [{
+				id: 'a', children: [{
+					id: 'aa'
+				}]
+			}]
+		};
+
+		const _: (id: string) => Element = find.bind(null, root.children);
+
+		const tree = new AsyncDataTree<Element, Element>('test', container, delegate, [renderer], dataSource, { identityProvider });
+		tree.layout(200);
+
+		const pSetInput = tree.setInput(root);
+		calls.pop()!(); // resolve getChildren(root)
+		await pSetInput;
+
+		const pExpandA = tree.expand(_('a'));
+		assert.equal(calls.length, 1, 'expand(a) should\'ve called getChildren(a)');
+
+		let race = await Promise.race([pExpandA.then(() => 'expand'), timeout(1).then(() => 'timeout')]);
+		assert.equal(race, 'timeout', 'expand(a) should not be yet done');
+
+		calls.pop()!();
+		assert.equal(calls.length, 0, 'no pending getChildren calls');
+
+		race = await Promise.race([pExpandA.then(() => 'expand'), timeout(1).then(() => 'timeout')]);
+		assert.equal(race, 'expand', 'expand(a) should now be done');
+	});
 });

src/vs/workbench/contrib/files/browser/views/explorerView.ts

@@ -50,7 +50,6 @@ import { first } from 'vs/base/common/arrays';
 import { withNullAsUndefined } from 'vs/base/common/types';
 import { IFileService, FileSystemProviderCapabilities } from 'vs/platform/files/common/files';
 import { dispose } from 'vs/base/common/lifecycle';
-import { timeout } from 'vs/base/common/async';
 
 export class ExplorerView extends ViewletPanel {
 	static readonly ID: string = 'workbench.explorer.fileView';
@@ -522,8 +521,6 @@ export class ExplorerView extends ViewletPanel {
 
 		while (item && item.resource.toString() !== resource.toString()) {
 			await this.tree.expand(item);
-			// Tree returns too early from the expand, need to wait for next tick #77106
-			await timeout(0);
 			item = first(values(item.children), i => isEqualOrParent(resource, i.resource));
 		}