fix: release terminal startup promises on dispose#319855
Open
vivekjm wants to merge 2 commits into
Open
Conversation
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @anthonykim1Matched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds disposal-time cleanup for TerminalInstance startup synchronization so callers don’t hang on pending barriers/promises after an instance is disposed.
Changes:
- Make internal startup promise/barriers nullable and provide safe fallbacks (
xtermReadyPromise, barrier waits). - Ensure disposal opens barriers and clears references to release waiters.
- Add a regression test verifying promises/barriers are released and
focusWhenReady()completes after dispose.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.ts | Adds a regression test that exercises dispose behavior around startup promises/barriers. |
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Clears startup sync primitives on dispose and makes waits resilient when disposed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #276610
Details
TerminalInstancekept per-instance startup promises and barriers after disposal. Creating and disposing terminals repeatedly could leave the frontend startup promise/barrier objects retained after the instance was already gone.This change opens any pending startup barriers during dispose, clears the xterm-ready promise and barrier references, and keeps the public readiness helpers safe after disposal by returning already-resolved fallback promises.
Evidence
Before: the issue repro reported retained promises growing with terminal create/delete cycles, with stacks pointing at
terminalInstance.tsstartup promise/barrier creation.After: the added regression test disposes a terminal instance, verifies
_xtermReadyPromise,_containerReadyBarrier, and_attachBarrierare cleared, and verifies post-disposextermReadyPromiseandfocusWhenReady()remain safe.Validation
npm ciin/tmp/vscode-pr-checkwith Node 24.15.0npm run compile-check-ts-native -- --pretty falsenpm run gulp compile-clientnpm run test-browser-no-install -- --browser chromium --run src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.ts --grep "release startup promises"npm run test-browser-no-install -- --browser chromium --run src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.ts(94 passing, 2 pending)node build/eslint.ts src/vs/workbench/contrib/terminal/browser/terminalInstance.ts src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.tsgit diff --check origin/main...HEAD