fix: resolve #305051 - fix listener leak on LanguageService.onDidChange#305186
fix: resolve #305051 - fix listener leak on LanguageService.onDidChange#305186maruthang wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…onDidChange Introduce a single shared observable for LanguageService.onDidChange that all LanguageSelection instances derive from, preventing unbounded listener accumulation when many models are open. Remove the unnecessary leakWarningThreshold override. Add regression tests.
|
@microsoft-github-policy-service agree |
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a listener leak in the editor’s LanguageService by ensuring LanguageSelection instances share a single underlying subscription to LanguageService.onDidChange, preventing unbounded listener growth when many models are open.
Changes:
- Introduces a shared
IObservableinLanguageServiceforonDidChangeand updatesLanguageSelectionto derive from it. - Adds regression tests to validate no listener leak, event propagation, and cleanup behavior.
- Adds a new
ProgressServicetest file and changeswithProgressbehavior forlocation === ''(appears unrelated to the PR’s stated goal).
Show a summary per file
| File | Description |
|---|---|
| src/vs/editor/common/services/languageService.ts | Centralizes language-change tracking into a single observable to avoid per-selection event subscriptions. |
| src/vs/editor/test/common/services/languageService.test.ts | Adds regression tests for listener leak prevention and cleanup semantics. |
| src/vs/workbench/services/progress/browser/progressService.ts | Adds special handling for empty string progress locations (unrelated to described LanguageService leak fix). |
| src/vs/workbench/services/progress/test/browser/progressService.test.ts | Adds coverage for withProgress string-location edge cases. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 4
| const originalHandler = errorHandler.getUnexpectedErrorHandler(); | ||
| errorHandler.setUnexpectedErrorHandler((e) => { | ||
| if (e instanceof ListenerLeakError) { | ||
| leakErrors.push(e); |
There was a problem hiding this comment.
The temporary unexpected error handler only records ListenerLeakErrors and silently drops any other unexpected errors. That can mask real failures occurring during this test run. Forward non-ListenerLeakError errors to the original handler (and still record leak errors) so unexpected errors are not swallowed.
| leakErrors.push(e); | |
| leakErrors.push(e); | |
| } else { | |
| originalHandler(e); |
|
|
||
| assert.ok(changeCount >= 0, 'onDidChange should have been called or not, depending on language resolution'); | ||
|
|
There was a problem hiding this comment.
This assertion is vacuous (changeCount >= 0 is always true), so the test doesn’t actually verify that LanguageSelection.onDidChange propagates changes. Make this test assert a concrete expected behavior (e.g. create a selection for a resource whose resolved language changes after registering a new language, and assert the event fires and/or selection.languageId updates).
| if (typeof location === 'string') { | ||
| if (location.length === 0) { | ||
| console.warn(`Bad progress location: empty string`); | ||
| return task({ report() { } }) as Promise<R>; | ||
| } |
There was a problem hiding this comment.
This new special-case for location === '' changes withProgress semantics (it now resolves instead of throwing) and adds a console.warn in a hot-path service. Also, this change appears unrelated to the PR’s stated goal (LanguageService listener leak). Consider removing this from the PR (or updating the PR description) and, if the behavior is needed, prefer consistent error handling over ad-hoc console logging (e.g. keep throwing Bad progress location or route through the existing error-handling/logging infrastructure).
|
|
||
| let taskExecuted = false; | ||
| const result = await progressService.withProgress( | ||
| { location: '' as any }, |
There was a problem hiding this comment.
location is already typed as string | ProgressLocation, so '' doesn’t require an as any cast here. Avoiding any keeps the test honest (and prevents accidental type escapes from spreading).
| { location: '' as any }, | |
| { location: '' }, |
Summary
Fixes #305051
Bug: A potential listener leak was detected in
textModelbecause eachLanguageSelectioncreated its ownobservableFromEventsubscription toLanguageService.onDidChange, accumulating hundreds of listeners when many models are open.Root Cause: Every call to
createLanguageSelection()registered a new listener on theonDidChangeemitter. With many open models, listener count grew unboundedly, triggering the leak warning.Fix: Introduced a single shared observable on
LanguageServicefor theonDidChangeevent. AllLanguageSelectioninstances now derive from this shared observable instead of each creating their own subscription. Also removed the unnecessaryleakWarningThresholdoverride that was masking the issue.Changes
src/vs/editor/common/services/languageService.ts: Replaced per-selectionobservableFromEventsubscriptions with a single shared observable that allLanguageSelectioninstances derive from. Removed theleakWarningThresholdoverride on theonDidChangeemitter.src/vs/editor/test/common/services/languageService.test.ts: Added 3 regression tests verifying that (1) only one listener is registered ononDidChangeregardless of model count, (2) language selections still react to language changes, and (3) disposing models properly cleans up.Testing
src/vs/editor/test/common/services/languageService.test.ts