-
Notifications
You must be signed in to change notification settings - Fork 37.6k
Description
Type: Bug
Version: 1.81.0
Commit: 6445d93
Date: 2023-08-02T12:36:11.334Z
Electron: 22.3.18
ElectronBuildId: 22689846
Chromium: 108.0.5359.215
Node.js: 16.17.1
V8: 10.8.168.25-electron.0
OS: Linux x64 5.15.0-76-generic
2023-08-06.18-41-06.mp4
Steps to Reproduce:
- Clear the "Test Results" Panel
- Execute command "Test: Run Tests in Current File" very quickly for several times
- The "Test Results" Panel now displays two terminals, with the top one acting like phantom, and the bottom one displaying the real messages.
After some investigation, I found the culprit to be that TestResultsViewContent.reveal is not concurrently safe.
vscode/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts
Lines 785 to 788 in 6445d93
| public async reveal(opts: { subject: InspectSubject; preserveFocus: boolean }) { | |
| this.didReveal.fire(opts); | |
| await Promise.all(this.contentProviders.map(p => p.update(opts.subject))); | |
| } |
Under certain cases, multiple calls to reveal() may happen concurrently within a short time period, which further leads to concurrent calls to TerminalMessagePeek.update, in which multiple instances of terminal are created and attached to the panel.
This bug was largely improved by commit 360b462 introduced by #189441 , which changed TestResultsViewContent.reveal into
vscode/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts
Lines 792 to 799 in 360b462
| public async reveal(opts: { subject: InspectSubject; preserveFocus: boolean }) { | |
| this.didReveal.fire(opts); | |
| if (!this.current || !equalsSubject(this.current, opts.subject)) { | |
| this.current = opts.subject; | |
| await Promise.all(this.contentProviders.map(p => p.update(opts.subject))); | |
| } | |
| } |
This change reduces the possibility of concurrently calling .update() to some extent and thus this bug may be harder to reproduce in the latest main branch. That being said, there's still a chance for this to happen. To thoroughly eliminate the risk, I fix it in PR #189756 .