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

Implements utils that can be used to ensure that all disposables are disposed after a test completes #127329

Merged
merged 7 commits into from Jul 7, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented Jun 28, 2021

This PR fixes #127250.
As part of this, some disposables that previously were automatically "disposed" by the garbage collector are now disposed explicitly.

Helpers are added to support tests in the following style:

suite('My Suite', () => {
    // Call this helper to ensure that all disposables are disposed after every test in this suite.
    // Internally, setup and teardown are used.
    ensureNoDisposablesAreLeakedInTestSuite();

    test('My Test 1 (succeeds)', () => {
        // This test succeeds, since all disposables are disposed.
        toDisposable(() => { }).dispose();
    });

    test('My Test 2 (fails)', () => {
        // This test fails, as the following disposable leaks.
        toDisposable(() => { });
    });
});

This also applies to event subscriptions that now use toSelfClearingDisposable, which supports tracking.
Use markAsSingleton to exclude disposable singletons (and all its registered children) that are created during the test.

@hediet hediet self-assigned this Jun 28, 2021
@hediet hediet requested review from alexdima, bpasero, mjbvz and jrieken and removed request for alexdima June 28, 2021 15:46
* Creates a disposable that calls `fn` and then clears its reference to `fn` when being disposed.
* This prevents memory leaks and ensures `fn` is called at most once.
*/
export function toNonLeakingDisposable(fn: () => void): IDisposable {
Copy link
Member

@alexdima alexdima Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why toDisposable should not do the same self.dispose = noop ? i.e. shouldn't toDisposable just be toNonLeakingDisposable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would change the semantics of toDisposable: Currently, multiple calls of dispose will lead to multiple calls of fn.
With self.dispose = noop, multiple calls of dispose will only call fn once.

I don't know if anyone depends on this behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jrieken @mjbvz That seems a wrong implementation of toDisposable. Could we try fixing that in the debt week (even outside this PR) to maximize our selfhosting coverage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @joaomoreno is Mr toDisposable but I agree that this should be called only once

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are all happy, optimistically pushed that: f8cf8f3

Copy link
Member

@joaomoreno joaomoreno Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @joaomoreno is Mr toDisposable but I agree that this should be called only once

As if I would have any reason other than didn't think about that 🤷‍♂️.

Let's have toDisposable do exactly what toNonLeakingDisposable does.

* Clears the value, but does not dispose it.
* The old value is returned.
*/
clearAndLeak(): T | undefined {
Copy link
Member

@alexdima alexdima Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find a usage of this. Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed in inline completions. Currently they use a copy of MutableDisposable that has this method, as I didn't want to be blocked by a code review.

Also, clearAndLeak makes use of the new tracking API (disposables can be removed from parent disposables).

src/vs/base/test/common/utils.ts Show resolved Hide resolved
@@ -19,18 +19,19 @@ export interface CancelablePromise<T> extends Promise<T> {
}

export function createCancelablePromise<T>(callback: (token: CancellationToken) => Promise<T>): CancelablePromise<T> {
const source = new CancellationTokenSource();
const disposableStore = new DisposableStore();
const source = disposableStore.add(new CancellationTokenSource());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why the source is wrapped in a dispo store? Is this to capture the listener from onCancellationRequested? This shouldn't be a problem when the store is always disposed. Btw, this made me realize that there is still a leak here: when cancellation happens (L26/27) the source/disposableStore isn't disposed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out the leak!

Is this to capture the listener from onCancellationRequested?

Yes.

This shouldn't be a problem when the store is always disposed.

You are right, it does not need to be disposed here as the disposables inside of CancellationTokenSource are not tracked.
It is very important to always dispose the event subscription though, otherwise the tooling introduced by this PR will recognize that as leak (even though garbage collection does take care of it).

@hediet hediet force-pushed the hediet/test-disposable-no-leak branch from 907f92b to dffe01c Compare July 5, 2021 10:35
@bpasero bpasero removed their request for review July 5, 2021 12:13
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for looking into this

@hediet hediet merged commit 4f11327 into main Jul 7, 2021
@hediet hediet deleted the hediet/test-disposable-no-leak branch July 7, 2021 07:48
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests should ensure that Disposables/Event subscriptions are disposed properly
5 participants