diff --git a/extensions/vscode-api-tests/package.json b/extensions/vscode-api-tests/package.json index 58ec85b1450..96ad4d41c5a 100644 --- a/extensions/vscode-api-tests/package.json +++ b/extensions/vscode-api-tests/package.json @@ -146,6 +146,11 @@ "id": "test.treeId", "name": "test-tree", "when": "never" + }, + { + "id": "test.treeSwitchUpdate", + "name": "test-tree-switch-update", + "when": "never" } ] }, diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts index a9d9bc5aa34..02259dc98c2 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts @@ -98,10 +98,11 @@ suite('vscode API - tree', () => { await provider.resolveNextRequest(); const [firstResult, secondResult] = await Promise.all([revealFirst, revealSecond]); - const error = firstResult.error ?? secondResult.error; - if (error && /Element with id .+ is already registered/.test(error.message)) { - assert.fail(error.message); - } + // Two concurrent root fetches race: the stale one gets invalidated and + // its reveal fails with "Cannot resolve". The other succeeds. + const errors = [firstResult.error, secondResult.error].filter((e): e is Error => !!e); + assert.strictEqual(errors.length, 1, 'Exactly one reveal should fail from the stale fetch'); + assert.ok(/Cannot resolve tree item/.test(errors[0].message), `Expected "Cannot resolve" error but got: ${errors[0].message}`); }); test('TreeView - element already registered after rapid root refresh', async function () { @@ -206,10 +207,113 @@ suite('vscode API - tree', () => { provider.resolveRequestWithElement(1, provider.getElement2()); const [firstResult, secondResult] = await Promise.all([firstReveal, secondReveal]); - const error = firstResult.error ?? secondResult.error; - if (error && /Element with id .+ is already registered/.test(error.message)) { - assert.fail(error.message); + const errors = [firstResult.error, secondResult.error].filter((e): e is Error => !!e); + assert.strictEqual(errors.length, 1, 'Exactly one reveal should fail from the stale fetch'); + assert.ok(/Cannot resolve tree item/.test(errors[0].message), `Expected "Cannot resolve" error but got: ${errors[0].message}`); + }); + + test('TreeView - element already registered during switch and update', async function () { + this.timeout(60_000); + + // This test reproduces a race condition where the tree is being "switched to" + // (via reveal, which triggers getChildren) while simultaneously the tree data + // is being updated with a new element being added. Both operations trigger + // concurrent getChildren calls. The first resolves with the old set of elements, + // the second resolves with a new set that includes a new element. If both try + // to register elements with the same ID, the error is thrown. + + type TreeElement = { readonly kind: 'leaf'; readonly instance: number }; + + class SwitchAndUpdateTreeDataProvider implements vscode.TreeDataProvider { + private readonly changeEmitter = new vscode.EventEmitter(); + private readonly requestEmitter = new vscode.EventEmitter(); + private readonly pendingRequests: DeferredPromise[] = []; + private readonly existingOld: TreeElement = { kind: 'leaf', instance: 1 }; + private readonly existingNew: TreeElement = { kind: 'leaf', instance: 2 }; + private readonly addedElement: TreeElement = { kind: 'leaf', instance: 3 }; + + readonly onDidChangeTreeData = this.changeEmitter.event; + + getChildren(element?: TreeElement): Thenable { + if (!element) { + const deferred = new DeferredPromise(); + this.pendingRequests.push(deferred); + this.requestEmitter.fire(this.pendingRequests.length); + return deferred.p; + } + return Promise.resolve([]); + } + + getTreeItem(element: TreeElement): vscode.TreeItem { + if (element === this.addedElement) { + const item = new vscode.TreeItem('added', vscode.TreeItemCollapsibleState.None); + item.id = 'added-elem'; + return item; + } + const item = new vscode.TreeItem('existing', vscode.TreeItemCollapsibleState.None); + item.id = 'existing-elem'; + return item; + } + + getParent(): TreeElement | undefined { + return undefined; + } + + async waitForRequestCount(count: number): Promise { + while (this.pendingRequests.length < count) { + await asPromise(this.requestEmitter.event); + } + } + + resolveRequestAt(index: number, elements: TreeElement[]): void { + const request = this.pendingRequests[index]; + if (request) { + request.complete(elements); + } + } + + getExistingOld(): TreeElement { return this.existingOld; } + getExistingNew(): TreeElement { return this.existingNew; } + getAddedElement(): TreeElement { return this.addedElement; } + + dispose(): void { + this.changeEmitter.dispose(); + this.requestEmitter.dispose(); + while (this.pendingRequests.length) { + this.pendingRequests.shift()!.complete([]); + } + } } + + const provider = new SwitchAndUpdateTreeDataProvider(); + disposables.push(provider); + + const treeView = vscode.window.createTreeView('test.treeSwitchUpdate', { treeDataProvider: provider }); + disposables.push(treeView); + + // Two concurrent reveals simulate the tree being "switched to" while also + // being updated: both trigger getChildren calls on the ext host directly. + const revealFirst = (treeView.reveal(provider.getExistingOld(), { expand: true }) + .then(() => ({ error: undefined as Error | undefined })) as Promise<{ error: Error | undefined }>) + .catch(error => ({ error })); + const revealSecond = (treeView.reveal(provider.getExistingNew(), { expand: true }) + .then(() => ({ error: undefined as Error | undefined })) as Promise<{ error: Error | undefined }>) + .catch(error => ({ error })); + + // Wait for both getChildren calls to be pending + await provider.waitForRequestCount(2); + + // Resolve first request with old data (just the existing element, old instance) + provider.resolveRequestAt(0, [provider.getExistingOld()]); + await delay(0); + + // Resolve second request with new data: different instance of existing + added element + provider.resolveRequestAt(1, [provider.getExistingNew(), provider.getAddedElement()]); + + const [firstResult, secondResult] = await Promise.all([revealFirst, revealSecond]); + const errors = [firstResult.error, secondResult.error].filter((e): e is Error => !!e); + assert.strictEqual(errors.length, 1, 'Exactly one reveal should fail from the stale fetch'); + assert.ok(/Cannot resolve tree item/.test(errors[0].message), `Expected "Cannot resolve" error but got: ${errors[0].message}`); }); test('TreeView - element already registered after refresh', async function () { @@ -345,9 +449,7 @@ suite('vscode API - tree', () => { await provider.resolveChildRequestAt(0, [staleChild]); const [firstResult, secondResult] = await Promise.all([firstReveal, secondReveal]); - const error = firstResult.error ?? secondResult.error; - if (error && /Element with id .+ is already registered/.test(error.message)) { - assert.fail(error.message); - } + assert.strictEqual(firstResult.error, undefined, `First reveal should not fail: ${firstResult.error?.message}`); + assert.strictEqual(secondResult.error, undefined, `Second reveal should not fail: ${secondResult.error?.message}`); }); }); diff --git a/src/vs/workbench/api/common/extHostTreeViews.ts b/src/vs/workbench/api/common/extHostTreeViews.ts index fce5e9d47db..c0d05cbb486 100644 --- a/src/vs/workbench/api/common/extHostTreeViews.ts +++ b/src/vs/workbench/api/common/extHostTreeViews.ts @@ -3,7 +3,6 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { localize } from '../../../nls.js'; import type * as vscode from 'vscode'; import { basename } from '../../../base/common/resources.js'; import { URI } from '../../../base/common/uri.js'; @@ -876,10 +875,14 @@ class ExtHostTreeView extends Disposable { if (duplicateHandle) { const existingElement = this._elements.get(duplicateHandle); if (existingElement) { - if (existingElement !== element) { - throw new Error(localize('treeView.duplicateElement', 'Element with id {0} is already registered', extTreeItem.id)); - } const existingNode = this._nodes.get(existingElement); + if (existingElement !== element) { + // A different element object was registered with the same ID. + // This can happen during concurrent tree operations (e.g., tree + // being switched to while data is updated). Clean up the stale + // element reference before re-registering with the new one. + this._nodes.delete(existingElement); + } if (existingNode) { const newNode = this._createTreeNode(element, extTreeItem, parentNode); this._updateNodeCache(element, newNode, existingNode, parentNode); diff --git a/src/vs/workbench/api/test/browser/extHostTreeViews.test.ts b/src/vs/workbench/api/test/browser/extHostTreeViews.test.ts index 751a4ff73f9..2ca97fac8a8 100644 --- a/src/vs/workbench/api/test/browser/extHostTreeViews.test.ts +++ b/src/vs/workbench/api/test/browser/extHostTreeViews.test.ts @@ -204,7 +204,7 @@ suite('ExtHostTreeView', function () { }); }); - test('error is thrown if id is not unique', (done) => { + test('duplicate id across siblings is handled gracefully', (done) => { tree['a'] = { 'aa': {}, }; @@ -212,7 +212,6 @@ suite('ExtHostTreeView', function () { 'aa': {}, 'ba': {} }; - let caughtExpectedError = false; store.add(target.onRefresh.event(() => { testObject.$getChildren('testNodeWithIdTreeProvider') .then(elements => { @@ -220,14 +219,54 @@ suite('ExtHostTreeView', function () { assert.deepStrictEqual(actuals, ['1/a', '1/b']); return testObject.$getChildren('testNodeWithIdTreeProvider', ['1/a']) .then(() => testObject.$getChildren('testNodeWithIdTreeProvider', ['1/b'])) - .then(() => assert.fail('Should fail with duplicate id')) - .catch(() => caughtExpectedError = true) - .finally(() => caughtExpectedError ? done() : assert.fail('Expected duplicate id error not thrown.')); - }); + .then(elements => { + // Children of 'b' should include both 'aa' and 'ba' + const children = unBatchChildren(elements)?.map(e => e.handle); + assert.deepStrictEqual(children, ['1/aa', '1/ba']); + done(); + }); + }).catch(done); })); onDidChangeTreeNode.fire(undefined); }); + test('different element instances with same id are replaced gracefully', async () => { + // Simulates the race condition: two concurrent getChildren calls return + // different element objects that map to the same tree item ID. The second + // call should replace the first's registration without error. + let callCount = 0; + const element1 = { key: 'x' }; + const element2 = { key: 'x' }; + + const treeView = testObject.createTreeView('testRaceProvider', { + treeDataProvider: { + getChildren: (): { key: string }[] => { + callCount++; + // Return a different object instance each time + return callCount === 1 ? [element1] : [element2]; + }, + getTreeItem: (element: { key: string }): TreeItem => { + return { label: { label: element.key }, id: 'same-id', collapsibleState: TreeItemCollapsibleState.None }; + }, + onDidChangeTreeData: onDidChangeTreeNode.event, + } + }, extensionsDescription); + + store.add(treeView); + + // First fetch — registers element1 with id 'same-id' + const first = await testObject.$getChildren('testRaceProvider'); + const firstChildren = unBatchChildren(first); + assert.strictEqual(firstChildren?.length, 1); + assert.strictEqual(firstChildren![0].handle, '1/same-id'); + + // Second fetch — different element instance, same id. Should not throw. + const second = await testObject.$getChildren('testRaceProvider'); + const secondChildren = unBatchChildren(second); + assert.strictEqual(secondChildren?.length, 1); + assert.strictEqual(secondChildren![0].handle, '1/same-id'); + }); + test('refresh root', function (done) { store.add(target.onRefresh.event(actuals => { assert.strictEqual(undefined, actuals);