Skip to content
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

Add DisposableStore for use in VS Code #74242

Closed
mjbvz opened this issue May 24, 2019 · 0 comments
Closed

Add DisposableStore for use in VS Code #74242

mjbvz opened this issue May 24, 2019 · 0 comments
Assignees
Labels
engineering VS Code - Build / issue tracking / etc.

Comments

@mjbvz
Copy link
Contributor

mjbvz commented May 24, 2019

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

@mjbvz mjbvz added the engineering VS Code - Build / issue tracking / etc. label May 24, 2019
@mjbvz mjbvz self-assigned this May 24, 2019
@mjbvz mjbvz closed this as completed in 3d5ce6b May 24, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

No branches or pull requests

1 participant