-
Notifications
You must be signed in to change notification settings - Fork 37.9k
Revert "fix: memory leak in abstract task service" #291306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "fix: memory leak in abstract task service" #291306
Conversation
This reverts commit d50d600.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR reverts a previous fix for a memory leak in the abstract task service (PR #289863) because that fix introduced a disposable leak warning for the ProblemReporter class. The revert addresses the immediate "LEAKED DISPOSABLE" error reported in issue #290128, but reintroduces the original memory leak problem where ProblemReporter's internal Emitter is never disposed.
Changes:
- Reverted ProblemReporter from extending Disposable to a plain class
- Removed DisposableMap-based tracking of workspace-specific disposables
- Changed event listener registration from workspace-scoped to service-level using
this._register()
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts:2647
- The ProblemReporter instance created here is never disposed, and the event listener registered with
this._register()is registered at the service level. This causes two issues:
- The ProblemReporter's internal Emitter is never disposed, leading to a memory leak
- The event listener is registered on the long-lived service instance, so it accumulates on each call to
_computeWorkspaceFolderTasks, which happens every time tasks are recomputed for workspace folders
A proper solution would be to either:
- Make ProblemReporter disposable and dispose it when the method completes, or
- Use a DisposableStore scoped to the method/workspace folder to manage both the ProblemReporter and its event subscription
const problemReporter = new ProblemReporter(this._outputChannel);
this._register(problemReporter.onDidError(error => this._showOutput(runSource, undefined, error)));
| class ProblemReporter implements TaskConfig.IProblemReporter { | ||
|
|
||
| private _validationStatus: ValidationStatus; | ||
| private readonly _onDidError: Emitter<string> = this._register(new Emitter<string>()); | ||
| private readonly _onDidError: Emitter<string> = new Emitter<string>(); | ||
| public readonly onDidError: Event<string> = this._onDidError.event; | ||
|
|
||
| constructor(private _outputChannel: IOutputChannel) { | ||
| super(); | ||
| this._validationStatus = new ValidationStatus(); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProblemReporter class has an Emitter that is never disposed, which will cause a memory leak. The Emitter should either be disposed explicitly or ProblemReporter should extend Disposable and register the Emitter for disposal using this._register().
While this revert fixes the immediate disposable leak issue, it reintroduces the original memory leak where the Emitter in ProblemReporter is not cleaned up. The ProblemReporter is created in _computeWorkspaceFolderTasks (line 2646) and _computeTasksForSingleConfig (line 2753), and these methods are called repeatedly as tasks are computed, leading to accumulated undisposed Emitters.
Reverts #289863
Fixes #290128