Revert "Fix element already registered (#281000)"#281278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reverts commit ca3f221, which attempted to fix a duplicate element registration issue in tree views. The revert removes sophisticated duplicate-handling logic and replaces it with a simpler check that has a race condition vulnerability.
Key changes:
- Removes complex duplicate element detection and re-registration logic from
_createAndRegisterTreeNode - Simplifies to a basic duplicate check that occurs after node creation but before cache addition
- Skips the test that validated the fix for concurrent tree element registrations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/api/common/extHostTreeViews.ts | Reverts _createAndRegisterTreeNode from handling duplicate elements gracefully to throwing an error, but introduces a race condition where concurrent registrations could both pass the duplicate check |
| extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts | Skips the test validating concurrent reveal() calls don't throw duplicate element errors, indicating this behavior is no longer supported |
| const node = this._createTreeNode(element, extTreeItem, parentNode); | ||
| if (extTreeItem.id && this._elements.has(node.item.handle)) { | ||
| throw new Error(localize('treeView.duplicateElement', 'Element with id {0} is already registered', extTreeItem.id)); | ||
| } |
There was a problem hiding this comment.
This duplicate check has a race condition. The check happens after the node is created (line 866) but before it's added to the cache (line 870). If two concurrent calls to _createAndRegisterTreeNode with the same element ID occur (e.g., two simultaneous reveal() calls as tested in tree.test.ts), both could pass this check before either adds the element to _elements, resulting in duplicate registrations.
The original code (that was reverted) handled this scenario by checking for duplicates before creating the node and had logic to handle re-registering the same element object. Consider either:
- Re-applying the reverted logic that properly handles concurrent registrations
- Adding synchronization to prevent concurrent registrations of the same element
- Checking for duplicates before creating the node to avoid the race window
| const node = this._createTreeNode(element, extTreeItem, parentNode); | |
| if (extTreeItem.id && this._elements.has(node.item.handle)) { | |
| throw new Error(localize('treeView.duplicateElement', 'Element with id {0} is already registered', extTreeItem.id)); | |
| } | |
| if (extTreeItem.id && this._elements.has(extTreeItem.id)) { | |
| throw new Error(localize('treeView.duplicateElement', 'Element with id {0} is already registered', extTreeItem.id)); | |
| } | |
| const node = this._createTreeNode(element, extTreeItem, parentNode); |
| }); | ||
|
|
||
| test('TreeView - element already registered', async function () { | ||
| test.skip('TreeView - element already registered', async function () { |
There was a problem hiding this comment.
Skipping this test instead of removing it suggests the tested behavior is still desired but currently broken. This test validates that concurrent reveal() calls for the same tree element don't throw duplicate element errors.
Since the main code change reintroduces the race condition this test was meant to verify is fixed, consider either:
- Removing this test entirely if the behavior is no longer supported
- Keeping it unskipped if the duplicate element handling will be fixed differently
- Adding a comment explaining why it's skipped and tracking the issue
This reverts commit ca3f221.