From fc2180e26d50b64f767b749634781e7d6975e0fd Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Mon, 7 Feb 2022 14:07:39 +0100 Subject: [PATCH 01/10] allow to pass disposable store into Event-utils --- src/vs/base/common/event.ts | 52 +++++++++++++++------------ src/vs/base/test/common/event.test.ts | 32 ++++++++++++++++- 2 files changed, 60 insertions(+), 24 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index a6fad59aa7bb1..9e0d386494fe5 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -52,25 +52,25 @@ export namespace Event { /** * @deprecated DO NOT use, this leaks memory */ - export function map(event: Event, map: (i: I) => O): Event { - return snapshot((listener, thisArgs = null, disposables?) => event(i => listener.call(thisArgs, map(i)), null, disposables)); + export function map(event: Event, map: (i: I) => O, disposable?: DisposableStore): Event { + return snapshot((listener, thisArgs = null, disposables?) => event(i => listener.call(thisArgs, map(i)), null, disposables), disposable); } /** * @deprecated DO NOT use, this leaks memory */ - export function forEach(event: Event, each: (i: I) => void): Event { - return snapshot((listener, thisArgs = null, disposables?) => event(i => { each(i); listener.call(thisArgs, i); }, null, disposables)); + export function forEach(event: Event, each: (i: I) => void, disposable?: DisposableStore): Event { + return snapshot((listener, thisArgs = null, disposables?) => event(i => { each(i); listener.call(thisArgs, i); }, null, disposables), disposable); } /** * @deprecated DO NOT use, this leaks memory */ - export function filter(event: Event, filter: (e: T | U) => e is T): Event; - export function filter(event: Event, filter: (e: T) => boolean): Event; - export function filter(event: Event, filter: (e: T | R) => e is R): Event; - export function filter(event: Event, filter: (e: T) => boolean): Event { - return snapshot((listener, thisArgs = null, disposables?) => event(e => filter(e) && listener.call(thisArgs, e), null, disposables)); + export function filter(event: Event, filter: (e: T | U) => e is T, disposable?: DisposableStore): Event; + export function filter(event: Event, filter: (e: T) => boolean, disposable?: DisposableStore): Event; + export function filter(event: Event, filter: (e: T | R) => e is R, disposable?: DisposableStore): Event; + export function filter(event: Event, filter: (e: T) => boolean, disposable?: DisposableStore): Event { + return snapshot((listener, thisArgs = null, disposables?) => event(e => filter(e) && listener.call(thisArgs, e), null, disposables), disposable); } /** @@ -93,19 +93,16 @@ export namespace Event { /** * @deprecated DO NOT use, this leaks memory */ - export function reduce(event: Event, merge: (last: O | undefined, event: I) => O, initial?: O): Event { + export function reduce(event: Event, merge: (last: O | undefined, event: I) => O, initial?: O, disposable?: DisposableStore): Event { let output: O | undefined = initial; return map(event, e => { output = merge(output, e); return output; - }); + }, disposable); } - /** - * @deprecated DO NOT use, this leaks memory - */ - function snapshot(event: Event): Event { + function snapshot(event: Event, disposable: DisposableStore | undefined): Event { let listener: IDisposable; const emitter = new Emitter({ onFirstListenerAdd() { @@ -116,6 +113,10 @@ export namespace Event { } }); + if (disposable) { + disposable.add(emitter); + } + return emitter.event; } @@ -151,15 +152,15 @@ export namespace Event { /** * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead */ - export function debounce(event: Event, merge: (last: T | undefined, event: T) => T, delay?: number, leading?: boolean, leakWarningThreshold?: number): Event; + export function debounce(event: Event, merge: (last: T | undefined, event: T) => T, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; /** * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead */ - export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay?: number, leading?: boolean, leakWarningThreshold?: number): Event; + export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; /** * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead */ - export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay: number = 100, leading = false, leakWarningThreshold?: number): Event { + export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay: number = 100, leading = false, leakWarningThreshold?: number, disposable?: DisposableStore): Event { let subscription: IDisposable; let output: O | undefined = undefined; @@ -196,13 +197,17 @@ export namespace Event { } }); + if (disposable) { + disposable.add(emitter); + } + return emitter.event; } /** * @deprecated DO NOT use, this leaks memory */ - export function latch(event: Event, equals: (a: T, b: T) => boolean = (a, b) => a === b): Event { + export function latch(event: Event, equals: (a: T, b: T) => boolean = (a, b) => a === b, disposable?: DisposableStore): Event { let firstCall = true; let cache: T; @@ -211,16 +216,16 @@ export namespace Event { firstCall = false; cache = value; return shouldEmit; - }); + }, disposable); } /** * @deprecated DO NOT use, this leaks memory */ - export function split(event: Event, isT: (e: T | U) => e is T): [Event, Event] { + export function split(event: Event, isT: (e: T | U) => e is T, disposable?: DisposableStore): [Event, Event] { return [ - Event.filter(event, isT), - Event.filter(event, e => !isT(e)) as Event, + Event.filter(event, isT, disposable), + Event.filter(event, e => !isT(e), disposable) as Event, ]; } @@ -274,6 +279,7 @@ export namespace Event { } export interface IChainableEvent { + event: Event; map(fn: (i: T) => O): IChainableEvent; forEach(fn: (i: T) => void): IChainableEvent; diff --git a/src/vs/base/test/common/event.test.ts b/src/vs/base/test/common/event.test.ts index a00f757f817f5..368711e9136db 100644 --- a/src/vs/base/test/common/event.test.ts +++ b/src/vs/base/test/common/event.test.ts @@ -7,7 +7,8 @@ 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 { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; +import { DisposableStore, IDisposable, isDisposable, toDisposable } from 'vs/base/common/lifecycle'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; namespace Samples { @@ -38,6 +39,35 @@ namespace Samples { } } +suite('Event utils dispose', function () { + + ensureNoDisposablesAreLeakedInTestSuite(); + + test('no leak with snapshot-utils', function () { + + const store = new DisposableStore(); + const emitter = new Emitter(); + const evens = Event.filter(emitter.event, n => n % 2 === 0, store); + let all = 0; + let leaked = evens(n => all += n); + assert.ok(isDisposable(leaked)); + emitter.dispose(); + store.dispose(); + }); + + test('no leak with debounce-util', function () { + const store = new DisposableStore(); + const emitter = new Emitter(); + const debounced = Event.debounce(emitter.event, (l) => 0, undefined, undefined, undefined, store); + + let all = 0; + let leaked = debounced(n => all += n); + assert.ok(isDisposable(leaked)); + emitter.dispose(); + store.dispose(); + }); +}); + suite('Event', function () { const counter = new Samples.EventCounter(); From b5155cdc56d71d904666934e4cae28acb6440263 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Mon, 7 Feb 2022 14:24:59 +0100 Subject: [PATCH 02/10] have mechanics in place to warn about public snapshotted events --- src/vs/base/common/event.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index 9e0d386494fe5..7d1e682636a51 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -103,11 +103,19 @@ export namespace Event { } function snapshot(event: Event, disposable: DisposableStore | undefined): Event { + // let stack = Stacktrace.create(); + // let count = 0; let listener: IDisposable; const emitter = new Emitter({ onFirstListenerAdd() { listener = event(emitter.fire, emitter); }, + // onListenerDidAdd() { + // if (++count === 2) { + // console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); + // stack.print(); + // } + // }, onLastListenerRemove() { listener.dispose(); } From d641293924eb8382c03bd28457f165c7e9541c2e Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Wed, 9 Feb 2022 18:10:10 +0100 Subject: [PATCH 03/10] reuse `DisposableTracker` to count leaked disposables --- src/vs/base/test/common/event.test.ts | 29 ++++++++++++++++++++++++--- src/vs/base/test/common/utils.ts | 15 ++++++++++++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/vs/base/test/common/event.test.ts b/src/vs/base/test/common/event.test.ts index 368711e9136db..9b301b2ef1d3d 100644 --- a/src/vs/base/test/common/event.test.ts +++ b/src/vs/base/test/common/event.test.ts @@ -7,8 +7,8 @@ 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 { DisposableStore, IDisposable, isDisposable, toDisposable } from 'vs/base/common/lifecycle'; -import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; +import { DisposableStore, IDisposable, isDisposable, setDisposableTracker, toDisposable } from 'vs/base/common/lifecycle'; +import { DisposableTracker } from 'vs/base/test/common/utils'; namespace Samples { @@ -41,30 +41,53 @@ namespace Samples { suite('Event utils dispose', function () { - ensureNoDisposablesAreLeakedInTestSuite(); + let tracker = new DisposableTracker(); + + function assertDisposablesCount(expected: number) { + assert.strictEqual(tracker.getTrackedDisposables().length, expected); + } + + setup(() => { + tracker = new DisposableTracker(); + setDisposableTracker(tracker); + }); + + teardown(function () { + setDisposableTracker(null); + }); test('no leak with snapshot-utils', function () { const store = new DisposableStore(); const emitter = new Emitter(); const evens = Event.filter(emitter.event, n => n % 2 === 0, store); + assertDisposablesCount(1); // snaphot only listen when `evens` is being listened on + let all = 0; let leaked = evens(n => all += n); assert.ok(isDisposable(leaked)); + assertDisposablesCount(3); + emitter.dispose(); store.dispose(); + assertDisposablesCount(1); // leaked is still there }); test('no leak with debounce-util', function () { const store = new DisposableStore(); const emitter = new Emitter(); const debounced = Event.debounce(emitter.event, (l) => 0, undefined, undefined, undefined, store); + assertDisposablesCount(1); // debounce only listens when `debounce` is being listened on let all = 0; let leaked = debounced(n => all += n); assert.ok(isDisposable(leaked)); + assertDisposablesCount(3); + emitter.dispose(); store.dispose(); + + assertDisposablesCount(1); // leaked is still there }); }); diff --git a/src/vs/base/test/common/utils.ts b/src/vs/base/test/common/utils.ts index bab167cd72cba..00ec54f29eed9 100644 --- a/src/vs/base/test/common/utils.ts +++ b/src/vs/base/test/common/utils.ts @@ -47,7 +47,7 @@ interface DisposableData { isSingleton: boolean; } -class DisposableTracker implements IDisposableTracker { +export class DisposableTracker implements IDisposableTracker { private readonly livingDisposables = new Map(); private getDisposableData(d: IDisposable) { @@ -90,6 +90,17 @@ class DisposableTracker implements IDisposableTracker { return result; } + getTrackedDisposables() { + const rootParentCache = new Map(); + + const leaking = [...this.livingDisposables.entries()] + .filter(([, v]) => v.source !== null && !this.getRootParent(v, rootParentCache).isSingleton) + .map(([k]) => k) + .flat(); + + return leaking; + } + ensureNoLeakingDisposables() { const rootParentCache = new Map(); const leaking = [...this.livingDisposables.values()] @@ -109,6 +120,7 @@ class DisposableTracker implements IDisposableTracker { throw new Error(`These disposables were not disposed:\n${s}`); } } + } /** @@ -148,4 +160,3 @@ export async function throwIfDisposablesAreLeakedAsync(body: () => Promise setDisposableTracker(null); tracker.ensureNoLeakingDisposables(); } - From db9a43608a4f61e29d46f7487027787c33c275c3 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 14:54:12 +0100 Subject: [PATCH 04/10] swap deprecation note with explainer --- src/vs/base/common/event.ts | 44 ++++++++++++------- src/vs/base/common/lifecycle.ts | 2 +- .../electron-sandbox/nativeHostService.ts | 2 +- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index f6aa1dc1f188a..ab883e5b9c934 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -50,21 +50,27 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function map(event: Event, map: (i: I) => O, disposable?: DisposableStore): Event { return snapshot((listener, thisArgs = null, disposables?) => event(i => listener.call(thisArgs, map(i)), null, disposables), disposable); } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function forEach(event: Event, each: (i: I) => void, disposable?: DisposableStore): Event { return snapshot((listener, thisArgs = null, disposables?) => event(i => { each(i); listener.call(thisArgs, i); }, null, disposables), disposable); } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function filter(event: Event, filter: (e: T | U) => e is T, disposable?: DisposableStore): Event; export function filter(event: Event, filter: (e: T) => boolean, disposable?: DisposableStore): Event; @@ -91,7 +97,9 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function reduce(event: Event, merge: (last: O | undefined, event: I) => O, initial?: O, disposable?: DisposableStore): Event { let output: O | undefined = initial; @@ -103,19 +111,19 @@ export namespace Event { } function snapshot(event: Event, disposable: DisposableStore | undefined): Event { - // let stack = Stacktrace.create(); - // let count = 0; + let stack = Stacktrace.create(); + let count = 0; let listener: IDisposable; const emitter = new Emitter({ onFirstListenerAdd() { listener = event(emitter.fire, emitter); }, - // onListenerDidAdd() { - // if (++count === 2) { - // console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); - // stack.print(); - // } - // }, + onListenerDidAdd() { + if (++count === 2) { + console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); + stack.print(); + } + }, onLastListenerRemove() { listener.dispose(); } @@ -213,7 +221,9 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function latch(event: Event, equals: (a: T, b: T) => boolean = (a, b) => a === b, disposable?: DisposableStore): Event { let firstCall = true; @@ -228,7 +238,9 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function split(event: Event, isT: (e: T | U) => e is T, disposable?: DisposableStore): [Event, Event] { return [ @@ -238,7 +250,9 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function buffer(event: Event, flushAfterTimeout = false, _buffer: T[] = []): Event { let buffer: T[] | null = _buffer.slice(); diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index 471d98b64878d..3283d07c57544 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -242,7 +242,7 @@ export abstract class Disposable implements IDisposable { static readonly None = Object.freeze({ dispose() { } }); - private readonly _store = new DisposableStore(); + protected readonly _store = new DisposableStore(); constructor() { trackDisposable(this); diff --git a/src/vs/workbench/services/host/electron-sandbox/nativeHostService.ts b/src/vs/workbench/services/host/electron-sandbox/nativeHostService.ts index 81576aadd15ef..cbe6beb7e38ed 100644 --- a/src/vs/workbench/services/host/electron-sandbox/nativeHostService.ts +++ b/src/vs/workbench/services/host/electron-sandbox/nativeHostService.ts @@ -43,7 +43,7 @@ class WorkbenchHostService extends Disposable implements IHostService { private _onDidChangeFocus: Event = Event.latch(Event.any( Event.map(Event.filter(this.nativeHostService.onDidFocusWindow, id => id === this.nativeHostService.windowId), () => this.hasFocus), Event.map(Event.filter(this.nativeHostService.onDidBlurWindow, id => id === this.nativeHostService.windowId), () => this.hasFocus) - )); + ), undefined, this._store); get hasFocus(): boolean { return document.hasFocus(); From 854309e8f25ca83c2bbbc0405e51f318c4da54ef Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:01:10 +0100 Subject: [PATCH 05/10] add TRACE_LIKEY_SNAPSHOT_LEAKAGE flag to trace leakage --- src/vs/base/common/event.ts | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index ab883e5b9c934..4b1f0342ee951 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -10,6 +10,9 @@ import { combinedDisposable, Disposable, DisposableStore, IDisposable, toDisposa import { LinkedList } from 'vs/base/common/linkedList'; import { StopWatch } from 'vs/base/common/stopwatch'; +let TRACE_LIKEY_SNAPSHOT_LEAKAGE = false; +// TRACE_LIKEY_SNAPSHOT_LEAKAGE = Boolean("true"); // causes an ESLint warning so that this isn't pushed by accident + /** * To an event a function with one or zero parameters * can be subscribed. The event is the subscriber function itself. @@ -111,23 +114,29 @@ export namespace Event { } function snapshot(event: Event, disposable: DisposableStore | undefined): Event { - let stack = Stacktrace.create(); - let count = 0; let listener: IDisposable; - const emitter = new Emitter({ + + const options: EmitterOptions | undefined = { onFirstListenerAdd() { listener = event(emitter.fire, emitter); }, - onListenerDidAdd() { + onLastListenerRemove() { + listener.dispose(); + } + }; + + if (TRACE_LIKEY_SNAPSHOT_LEAKAGE) { + let stack = Stacktrace.create(); + let count = 0; + options.onListenerDidAdd = () => { if (++count === 2) { console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); stack.print(); } - }, - onLastListenerRemove() { - listener.dispose(); - } - }); + }; + } + + const emitter = new Emitter(options); if (disposable) { disposable.add(emitter); From 63cb496df69265681cf72605d85c57f60faad1b0 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:03:19 +0100 Subject: [PATCH 06/10] Revert "add `Event.debouncedListener` as replacement for `debounce`, https://github.com/microsoft/vscode/issues/123487" This reverts commit f41273cd7852866dc72dd9430dd289ef62d97b4b. --- src/vs/base/common/event.ts | 35 ++----------------- .../notebook/browser/notebook.contribution.ts | 5 ++- 2 files changed, 5 insertions(+), 35 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index 4b1f0342ee951..8fc6824e2bf02 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -145,45 +145,16 @@ export namespace Event { return emitter.event; } - export function debouncedListener(event: Event, listener: (data: O) => any, merge: (last: O | undefined, event: T) => O, delay: number = 100, leading: boolean = false): IDisposable { - - let output: O | undefined = undefined; - let handle: any = undefined; - let numDebouncedCalls = 0; - - return event(cur => { - numDebouncedCalls++; - output = merge(output, cur); - - if (leading && !handle) { - listener(output); - output = undefined; - } - - clearTimeout(handle); - handle = setTimeout(() => { - const _output = output; - output = undefined; - handle = undefined; - if (!leading || numDebouncedCalls > 1) { - listener(_output!); - } - - numDebouncedCalls = 0; - }, delay); - }); - } - /** - * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead + * @deprecated DO NOT use, this leaks memory */ export function debounce(event: Event, merge: (last: T | undefined, event: T) => T, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; /** - * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead + * @deprecated DO NOT use, this leaks memory */ export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; /** - * @deprecated this leaks memory, {@link debouncedListener} or {@link DebounceEmitter} instead + * @deprecated DO NOT use, this leaks memory */ export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay: number = 100, leading = false, leakWarningThreshold?: number, disposable?: DisposableStore): Event { diff --git a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts index 4ae1c756c537a..1b066da010402 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts @@ -507,12 +507,11 @@ class NotebookEditorManager implements IWorkbenchContribution { // OPEN notebook editor for models that have turned dirty without being visible in an editor type E = IResolvedNotebookEditorModel; - this._disposables.add(Event.debouncedListener( + this._disposables.add(Event.debounce( this._notebookEditorModelService.onDidChangeDirty, - this._openMissingDirtyNotebookEditors.bind(this), (last, current) => !last ? [current] : [...last, current], 100 - )); + )(this._openMissingDirtyNotebookEditors, this)); // CLOSE notebook editor for models that have no more serializer this._disposables.add(notebookService.onWillRemoveViewType(e => { From 447887fae927558e17a791d1ae6785ce0560cac5 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:04:25 +0100 Subject: [PATCH 07/10] add usage-warning to debounce-util --- src/vs/base/common/event.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index 8fc6824e2bf02..f685c885d1bb9 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -146,16 +146,18 @@ export namespace Event { } /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function debounce(event: Event, merge: (last: T | undefined, event: T) => T, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; /** - * @deprecated DO NOT use, this leaks memory + * *NOTE* that this function returns an `Event` and it MUST be called with a `DisposableStore` whenever the returned + * event is accessible to "third parties", e.g the event is a public property. Otherwise a leaked listener on the + * returned event causes this utility to leak a listener on the original event. */ export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay?: number, leading?: boolean, leakWarningThreshold?: number, disposable?: DisposableStore): Event; - /** - * @deprecated DO NOT use, this leaks memory - */ + export function debounce(event: Event, merge: (last: O | undefined, event: I) => O, delay: number = 100, leading = false, leakWarningThreshold?: number, disposable?: DisposableStore): Event { let subscription: IDisposable; From 665a9c61a84e64974d7eb8644f69d01e06e16682 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:10:20 +0100 Subject: [PATCH 08/10] extract `_addLeakageTraceLogic`-util --- src/vs/base/common/event.ts | 40 +++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index f685c885d1bb9..1c49d6e57bd2c 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -10,8 +10,23 @@ import { combinedDisposable, Disposable, DisposableStore, IDisposable, toDisposa import { LinkedList } from 'vs/base/common/linkedList'; import { StopWatch } from 'vs/base/common/stopwatch'; -let TRACE_LIKEY_SNAPSHOT_LEAKAGE = false; -// TRACE_LIKEY_SNAPSHOT_LEAKAGE = Boolean("true"); // causes an ESLint warning so that this isn't pushed by accident + +function _addLeakageTraceLogic(options: EmitterOptions) { + let enabled = false; + // enabled = Boolean("true"); // causes an ESLint warning so that this isn't pushed by accident + if (enabled) { + const { onListenerDidAdd: origListenerDidAdd } = options; + const stack = Stacktrace.create(); + let count = 0; + options.onListenerDidAdd = () => { + if (++count === 2) { + console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); + stack.print(); + } + origListenerDidAdd?.(); + }; + } +} /** * To an event a function with one or zero parameters @@ -125,15 +140,8 @@ export namespace Event { } }; - if (TRACE_LIKEY_SNAPSHOT_LEAKAGE) { - let stack = Stacktrace.create(); - let count = 0; - options.onListenerDidAdd = () => { - if (++count === 2) { - console.warn('snapshotted emitter LIKELY used public and SHOULD HAVE BEEN created with DisposableStore. snapshotted here'); - stack.print(); - } - }; + if (!disposable) { + _addLeakageTraceLogic(options); } const emitter = new Emitter(options); @@ -165,7 +173,7 @@ export namespace Event { let handle: any = undefined; let numDebouncedCalls = 0; - const emitter = new Emitter({ + const options: EmitterOptions | undefined = { leakWarningThreshold, onFirstListenerAdd() { subscription = event(cur => { @@ -193,7 +201,13 @@ export namespace Event { onLastListenerRemove() { subscription.dispose(); } - }); + }; + + if (!disposable) { + _addLeakageTraceLogic(options); + } + + const emitter = new Emitter(options); if (disposable) { disposable.add(emitter); From 3e272359d786104b6942da0e5701fe105be62964 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:30:24 +0100 Subject: [PATCH 09/10] 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 1c49d6e57bd2c..74c239af6a8e1 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 3283d07c57544..85ef9a3efffb5 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 9b301b2ef1d3d..7eab340491d5c 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 68f754b39531a..9114d78082017 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', () => { }); }); }); - From e8cf177b98b76030ff803e18fa36f84daa733369 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 11 Feb 2022 15:35:41 +0100 Subject: [PATCH 10/10] VoidFunction isn't as useful --- src/vs/base/common/lifecycle.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index 85ef9a3efffb5..c67bb11670053 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -345,8 +345,8 @@ export class RefCountedDisposable { */ export class SafeDisposable implements IDisposable { - dispose: VoidFunction = () => { }; - unset: VoidFunction = () => { }; + dispose: () => void = () => { }; + unset: () => void = () => { }; isset: () => boolean = () => false; constructor() {