Skip to content

Add DisposableStore for use in VS Code #74242

@mjbvz

Description

@mjbvz

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:

  1. new Foo()
  2. We then call Foo.dispose() before the async part of doStuff as 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

Metadata

Metadata

Assignees

Labels

engineeringVS Code - Build / issue tracking / etc.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions