Revert "fix: memory leak in abstract task service"#291306
Conversation
This reverts commit d50d600.
There was a problem hiding this comment.
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(); | ||
| } |
There was a problem hiding this comment.
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