From 3e272359d786104b6942da0e5701fe105be62964 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:30:24 +0100 Subject: [PATCH] improve `SafeDisposable` and move to lifecyle --- src/vs/base/common/event.ts | 28 ++-------------- src/vs/base/common/lifecycle.ts | 29 ++++++++++++++++ src/vs/base/test/common/event.test.ts | 41 ++++++++++------------- src/vs/base/test/common/lifecycle.test.ts | 22 ++++++++++-- 4 files changed, 69 insertions(+), 51 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index 1c49d6e57bd..74c239af6a8 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -6,7 +6,7 @@ import { CancellationToken } from 'vs/base/common/cancellation'; import { onUnexpectedError } from 'vs/base/common/errors'; import { once as onceFn } from 'vs/base/common/functional'; -import { combinedDisposable, Disposable, DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; +import { combinedDisposable, Disposable, DisposableStore, IDisposable, SafeDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { LinkedList } from 'vs/base/common/linkedList'; import { StopWatch } from 'vs/base/common/stopwatch'; @@ -546,28 +546,6 @@ class Stacktrace { } } -export class SafeDisposable implements IDisposable { - - private static _noop = () => { }; - - dispose: () => void = SafeDisposable._noop; - unset: () => void = SafeDisposable._noop; - isset: () => boolean = () => false; - - set(disposable: IDisposable) { - let actual: IDisposable | undefined = disposable; - this.unset = () => actual = undefined; - this.isset = () => actual !== undefined; - this.dispose = () => { - if (actual) { - actual.dispose(); - actual = undefined; - } - }; - return this; - } -} - class Listener { readonly subscription = new SafeDisposable(); @@ -655,7 +633,7 @@ export class Emitter { this._options.onListenerDidAdd(this, callback, thisArgs); } - const result = listener.subscription.set(toDisposable(() => { + const result = listener.subscription.set(() => { if (removeMonitor) { removeMonitor(); } @@ -668,7 +646,7 @@ export class Emitter { } } } - })); + }); if (disposables instanceof DisposableStore) { disposables.add(result); diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index 3283d07c575..85ef9a3efff 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -339,6 +339,35 @@ export class RefCountedDisposable { } } +/** + * A safe disposable can be `unset` so that a leaked reference (listener) + * can be cut-off. + */ +export class SafeDisposable implements IDisposable { + + dispose: VoidFunction = () => { }; + unset: VoidFunction = () => { }; + isset: () => boolean = () => false; + + constructor() { + trackDisposable(this); + } + + set(fn: Function) { + let callback: Function | undefined = fn; + this.unset = () => callback = undefined; + this.isset = () => callback !== undefined; + this.dispose = () => { + if (callback) { + callback(); + callback = undefined; + markAsDisposed(this); + } + }; + return this; + } +} + export interface IReference extends IDisposable { readonly object: T; } diff --git a/src/vs/base/test/common/event.test.ts b/src/vs/base/test/common/event.test.ts index 9b301b2ef1d..7eab340491d 100644 --- a/src/vs/base/test/common/event.test.ts +++ b/src/vs/base/test/common/event.test.ts @@ -6,7 +6,7 @@ import * as assert from 'assert'; import { timeout } from 'vs/base/common/async'; import { CancellationToken } from 'vs/base/common/cancellation'; import { errorHandler, setUnexpectedErrorHandler } from 'vs/base/common/errors'; -import { AsyncEmitter, DebounceEmitter, Emitter, Event, EventBufferer, EventMultiplexer, IWaitUntil, MicrotaskEmitter, PauseableEmitter, Relay, SafeDisposable } from 'vs/base/common/event'; +import { AsyncEmitter, DebounceEmitter, Emitter, Event, EventBufferer, EventMultiplexer, IWaitUntil, MicrotaskEmitter, PauseableEmitter, Relay } from 'vs/base/common/event'; import { DisposableStore, IDisposable, isDisposable, setDisposableTracker, toDisposable } from 'vs/base/common/lifecycle'; import { DisposableTracker } from 'vs/base/test/common/utils'; @@ -43,8 +43,20 @@ suite('Event utils dispose', function () { let tracker = new DisposableTracker(); - function assertDisposablesCount(expected: number) { - assert.strictEqual(tracker.getTrackedDisposables().length, expected); + function assertDisposablesCount(expected: number | Array) { + if (Array.isArray(expected)) { + const instances = new Set(expected); + const actualInstances = tracker.getTrackedDisposables(); + assert.strictEqual(actualInstances.length, expected.length); + + for (let item of actualInstances) { + assert.ok(instances.has(item)); + } + + } else { + assert.strictEqual(tracker.getTrackedDisposables().length, expected); + } + } setup(() => { @@ -70,7 +82,7 @@ suite('Event utils dispose', function () { emitter.dispose(); store.dispose(); - assertDisposablesCount(1); // leaked is still there + assertDisposablesCount([leaked]); // leaked is still there }); test('no leak with debounce-util', function () { @@ -87,7 +99,7 @@ suite('Event utils dispose', function () { emitter.dispose(); store.dispose(); - assertDisposablesCount(1); // leaked is still there + assertDisposablesCount([leaked]); // leaked is still there }); }); @@ -373,25 +385,6 @@ suite('Event', function () { const dispo = e.event(() => { }); dispo.dispose.call(undefined); // assert that disposable can be called with this }); - - test('SafeDisposable, dispose', function () { - let disposed = 0; - const actual = toDisposable(() => disposed += 1); - const d = new SafeDisposable(); - d.set(actual); - d.dispose(); - assert.strictEqual(disposed, 1); - }); - - test('SafeDisposable, unset', function () { - let disposed = 0; - const actual = toDisposable(() => disposed += 1); - const d = new SafeDisposable(); - d.set(actual); - d.unset(); - d.dispose(); - assert.strictEqual(disposed, 0); - }); }); suite('AsyncEmitter', function () { diff --git a/src/vs/base/test/common/lifecycle.test.ts b/src/vs/base/test/common/lifecycle.test.ts index 68f754b3953..9114d780820 100644 --- a/src/vs/base/test/common/lifecycle.test.ts +++ b/src/vs/base/test/common/lifecycle.test.ts @@ -5,7 +5,7 @@ import * as assert from 'assert'; import { Emitter } from 'vs/base/common/event'; -import { DisposableStore, dispose, IDisposable, markAsSingleton, MultiDisposeError, ReferenceCollection, toDisposable } from 'vs/base/common/lifecycle'; +import { DisposableStore, dispose, IDisposable, markAsSingleton, MultiDisposeError, ReferenceCollection, SafeDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { ensureNoDisposablesAreLeakedInTestSuite, throwIfDisposablesAreLeaked } from 'vs/base/test/common/utils'; class Disposable implements IDisposable { @@ -107,6 +107,25 @@ suite('Lifecycle', () => { let setValues2 = dispose(setValues); assert.ok(setValues === setValues2); }); + + test('SafeDisposable, dispose', function () { + let disposed = 0; + const actual = () => disposed += 1; + const d = new SafeDisposable(); + d.set(actual); + d.dispose(); + assert.strictEqual(disposed, 1); + }); + + test('SafeDisposable, unset', function () { + let disposed = 0; + const actual = () => disposed += 1; + const d = new SafeDisposable(); + d.set(actual); + d.unset(); + d.dispose(); + assert.strictEqual(disposed, 0); + }); }); suite('DisposableStore', () => { @@ -259,4 +278,3 @@ suite('No Leakage Utilities', () => { }); }); }); -