Skip to content

Commit

Permalink
eagerly create stacktrace value, avoid creating stacktraces, ignore t…
Browse files Browse the repository at this point in the history 
…he first 30 listener for leak checks, some lipstick, #143111
  • Loading branch information
@jrieken
jrieken committed Feb 21, 2022
1 parent 94f508d commit 26151a2
Showing 1 changed file with 73 additions and 59 deletions.
132 changes: 73 additions & 59 deletions src/vs/base/common/event.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,19 @@ import { LinkedList } from 'vs/base/common/linkedList';
import { StopWatch } from 'vs/base/common/stopwatch';


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?.();
};
}
}
// -----------------------------------------------------------------------------------------------------------------------
// Uncomment the next line to print warnings whenever an emitter with listeners is disposed. That is a sign of code smell.
// -----------------------------------------------------------------------------------------------------------------------
let _enableDisposeWithListenerWarning = false;
// _enableDisposeWithListenerWarning = Boolean("TRUE"); // causes a linter warning so that it cannot be pushed


// -----------------------------------------------------------------------------------------------------------------------
// Uncomment the next line to print warnings whenever a snapshotted event is used repeatedly without cleanup.
// See https://github.com/microsoft/vscode/issues/142851
// -----------------------------------------------------------------------------------------------------------------------
let _enableSnapshotPotentialLeakWarning = false;
// _enableSnapshotPotentialLeakWarning = Boolean("TRUE"); // causes a linter warning so that it cannot be pushed

/**
* To an event a function with one or zero parameters
Expand All @@ -39,6 +36,23 @@ export interface Event<T> {
export namespace Event {
export const None: Event<any> = () => Disposable.None;


function _addLeakageTraceLogic(options: EmitterOptions) {
if (_enableSnapshotPotentialLeakWarning) {
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?.();
};
}
}


/**
* Given an event, returns another event which only fires once.
*/
Expand Down Expand Up @@ -531,15 +545,10 @@ class LeakageMonitor {
class Stacktrace {

static create() {
return new Stacktrace(new Error());
return new Stacktrace(new Error().stack ?? '');
}

private constructor(private readonly _error: Error) { }

get value() {
// only access the stack late
return this._error.stack ?? '';
}
private constructor(readonly value: string) { }

print() {
console.warn(this.value.split('\n').slice(2).join('\n'));
Expand Down Expand Up @@ -583,6 +592,7 @@ class Listener<T> {
}
*/
export class Emitter<T> {

private readonly _options?: EmitterOptions;
private readonly _leakageMon?: LeakageMonitor;
private readonly _perfMon?: EventProfiling;
Expand All @@ -597,6 +607,41 @@ export class Emitter<T> {
this._perfMon = this._options?._profName ? new EventProfiling(this._options._profName) : undefined;
}

dispose() {
if (!this._disposed) {
this._disposed = true;

// It is bad to have listeners at the time of disposing an emitter, it is worst to have listeners keep the emitter
// alive via the reference that's embedded in their disposables. Therefore we loop over all remaining listeners and
// unset their subscriptions/disposables. Looping and blaming remaining listeners is done on next tick because the
// the following programming pattern is very popular:
//
// const someModel = this._disposables.add(new ModelObject()); // (1) create and register model
// this._disposables.add(someModel.onDidChange(() => { ... }); // (2) subscribe and register model-event listener
// ...later...
// this._disposables.dispose(); disposes (1) then (2): don't warn after (1) but after the "overall dispose" is done

if (this._listeners) {
if (_enableDisposeWithListenerWarning) {
const listeners = Array.from(this._listeners);
queueMicrotask(() => {
for (const listener of listeners) {
if (listener.subscription.isset()) {
listener.subscription.unset();
listener.stack?.print();
}
}
});
}

this._listeners.clear();
}
this._deliveryQueue?.clear();
this._options?.onLastListenerRemove?.();
this._leakageMon?.dispose();
}
}

/**
* For the public to allow to subscribe
* to events from this Emitter
Expand All @@ -616,12 +661,16 @@ export class Emitter<T> {

let removeMonitor: Function | undefined;
let stack: Stacktrace | undefined;
if (this._leakageMon) {
if (this._leakageMon && this._listeners.size >= 30) {
// check and record this emitter for potential leakage
stack = Stacktrace.create();
removeMonitor = this._leakageMon.check(stack, this._listeners.size + 1);
}

if (_enableDisposeWithListenerWarning) {
stack = stack ?? Stacktrace.create();
}

const listener = new Listener(callback, thisArgs, stack);
const removeListener = this._listeners.push(listener);

Expand Down Expand Up @@ -700,41 +749,6 @@ export class Emitter<T> {
}
return (!this._listeners.isEmpty());
}

dispose() {
if (!this._disposed) {
this._disposed = true;

// It is bad to have listeners at the time of disposing an emitter, it is worst to have listeners keep the emitter
// alive via the reference that's embedded their disposables. Therefore we loop over all remaining listeners and
// unset their subscriptions/disposables. Looping and blaming remaining listeners is done on next tick because the
// the following programming pattern is very popular:
//
// const someModel = this._disposables.add(new ModelObject()); // (1) create and register model
// this._disposables.add(someModel.onDidChange(() => { ... }); // (2) subscribe and register model-event listener
// ...later...
// this._disposables.dispose(); disposes (1) then (2): don't warn after (1) but after the "overall dispose" is done

if (this._listeners) {
const listeners = Array.from(this._listeners);
this._listeners.clear();

queueMicrotask(() => {
for (const listener of listeners) {
if (listener.subscription.isset()) {
listener.subscription.unset();
// enable this to blame listeners that are still here
// listener.stack?.print();
}
}
});

}
this._deliveryQueue?.clear();
this._options?.onLastListenerRemove?.();
this._leakageMon?.dispose();
}
}
}


Expand Down

0 comments on commit 26151a2

Please sign in to comment.