-
Notifications
You must be signed in to change notification settings - Fork 38.2k
Description
Bug
Consider the following class that tries to manage a list of disposables:
class Foo {
private _disposables: Disposable[];
constructor() {
this._disposables.push(thing);
doStuff();
}
dispose() {
dispose(this._disposables);
}
private async doStuff(): Promise<void> {
await goDoTheStuff();
this._disposables.push(otherThing);
}
}This can leak a disposable in the following case:
new Foo()- We then call
Foo.dispose()before the async part ofdoStuffas finished
This exact pattern varies in our codebase, but it can be hit anytime we are adding to a disposable array inside of an async method (it can be hit sync too if you introduce a weird calling pattern)
Proposed fix
To fix this, I propose encapsulating the idea of set of disposables in a class that manages an array of disposables. If an instance of this class is disposed, any attempts to register a new disposable with it would have no effect and the registered object would be disposed
We basically already have this, but the current class is only intended to be used as base class. My proposal is to extract and expose a new value class from the current Disposable class