improve SafeDisposable and move to lifecyle

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<T> {
 
 	readonly subscription = new SafeDisposable();
@@ -655,7 +633,7 @@ export class Emitter<T> {
 					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<T> {
 							}
 						}
 					}
-				}));
+				});
 
 				if (disposables instanceof DisposableStore) {
 					disposables.add(result);

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<T> extends IDisposable {
 	readonly object: T;
 }

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,10 +43,22 @@ suite('Event utils dispose', function () {
 
 	let tracker = new DisposableTracker();
 
-	function assertDisposablesCount(expected: number) {
+	function assertDisposablesCount(expected: number | Array<IDisposable>) {
+		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(() => {
 		tracker = new DisposableTracker();
 		setDisposableTracker(tracker);
@@ -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 () {

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', () => {
 		});
 	});
 });
-