Fix listener leak in FocusSessionActionViewItem#311758
Closed
roblourens wants to merge 3 commits intomainfrom
Closed
Fix listener leak in FocusSessionActionViewItem#311758roblourens wants to merge 3 commits intomainfrom
roblourens wants to merge 3 commits intomainfrom
Conversation
Listeners added to debug sessions (onDidChangeName/onDidEndAdapter) in the constructor were not tied to the view item's lifetime. Since debug sessions outlive the action view item (recreated on toolbar updates), each recreation piled more listeners onto the same sessions, eventually triggering the 'potential listener LEAK detected' warning. Route all per-session listeners through a DisposableStore owned by the view item so they are released when the view item is disposed, while still self-cleaning when the session ends. Fixes #308534 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use sessionListenersStore.delete() instead of sessionListeners.dispose() so that ended sessions don't accumulate disposed stores in the parent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a listener leak in FocusSessionActionViewItem (Debug / REPL toolbar session selector) by ensuring per-debug-session listeners are disposed with the view item’s lifetime, preventing listener accumulation across action view item re-creations.
Changes:
- Introduces a
DisposableStoreowned byFocusSessionActionViewItemto own all per-session listeners. - Registers per-session
onDidChangeName/onDidEndAdapterlisteners via per-session child stores. - Removes per-session listener stores from the parent store when sessions end to avoid growth over time.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts | Routes session listeners through a view-item-owned DisposableStore and cleans up per-session stores on adapter end to prevent listener leaks. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
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 #308534 (Written by Copilot)
Listeners added to debug sessions (
onDidChangeName/onDidEndAdapter) inFocusSessionActionViewItem's constructor were not tied to the view item's lifetime. Since debug sessions outlive the action view item (it gets recreated on toolbar / title-area updates), each recreation piled more listeners onto the same long-lived sessions, eventually tripping the "potential listener LEAK detected, popular" warning from the REPL title toolbar.Fix: route all per-session listeners through a
DisposableStoreowned by the view item, so they're released when the view item is disposed — while still self-cleaning when the session ends (and removing themselves from the parent store so disposed children don't accumulate).(Recreated from #311745, which was closed because some CI jobs got stuck.)