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.

Sign up for GitHub

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

Fix listener leak for timers/promises #497

Merged
merged 1 commit into from
Aug 23, 2024
Merged

Fix listener leak for timers/promises #497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 74 additions & 33 deletions src/fake-timers-src.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
/**
* Queues a function to be called during a browser's idle periods
* @callback RequestIdleCallback
* @param {function(IdleDeadline)} callback

Check warning on line 27 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Syntax error in type: function(IdleDeadline)
* @param {{timeout: number}} options - an options object
* @returns {number} the id
*/
Expand All @@ -51,13 +51,13 @@

/**
* @typedef RequestAnimationFrame
* @property {function(number):void} requestAnimationFrame

Check warning on line 54 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @Property "requestAnimationFrame" description
* @returns {number} - the id
*/

/**
* @typedef Performance
* @property {function(): number} now

Check warning on line 60 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @Property "now" description
*/

/* eslint-disable jsdoc/require-property-description */
Expand Down Expand Up @@ -99,6 +99,7 @@
* @property {boolean} [shouldClearNativeTimers] inherited from config
* @property {{methodName:string, original:any}[] | undefined} timersModuleMethods
* @property {{methodName:string, original:any}[] | undefined} timersPromisesModuleMethods
* @property {Map<function(): void, AbortSignal>} abortListenerMap
*/
/* eslint-enable jsdoc/require-property-description */

Expand Down Expand Up @@ -328,7 +329,7 @@
return timer && timer.callAt >= from && timer.callAt <= to;
}

/**

Check warning on line 332 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @returns declaration
* @param {Clock} clock
* @param {Timer} job
*/
Expand Down Expand Up @@ -678,7 +679,7 @@
}

/* eslint consistent-return: "off" */
/**

Check warning on line 682 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

JSDoc @returns declaration present but return expression not available in function
* Timer comparitor
* @param {Timer} a
* @param {Timer} b
Expand Down Expand Up @@ -809,7 +810,7 @@
}
}

/**

Check warning on line 813 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @returns declaration
* Gets clear handler name for a given timer type
* @param {string} ttype
*/
Expand All @@ -820,7 +821,7 @@
return `clear${ttype}`;
}

/**

Check warning on line 824 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @returns declaration
* Gets schedule handler name for a given timer type
* @param {string} ttype
*/
Expand All @@ -831,7 +832,7 @@
return `set${ttype}`;
}

/**

Check warning on line 835 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @returns declaration
* Creates an anonymous function to warn only once
*/
function createWarnOnce() {
Expand All @@ -843,7 +844,7 @@
}
const warnOnce = createWarnOnce();

/**

Check warning on line 847 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @returns declaration
* @param {Clock} clock
* @param {number} timerId
* @param {string} ttype
Expand Down Expand Up @@ -967,6 +968,11 @@
// Prevent multiple executions which will completely remove these props
clock.methods = [];

for (const [listener, signal] of clock.abortListenerMap.entries()) {
signal.removeEventListener("abort", listener);
clock.abortListenerMap.delete(listener);
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
}

// return pending timers, to enable checking what timers remained on uninstall
if (!clock.timers) {
return [];
Expand Down Expand Up @@ -1042,7 +1048,7 @@

/**
* @typedef {object} Timers
* @property {setTimeout} setTimeout

Check warning on line 1051 in src/fake-timers-src.js

View workflow job for this annotation

GitHub Actions / lint 

Missing JSDoc @Property "setTimeout" description
* @property {clearTimeout} clearTimeout
* @property {setInterval} setInterval
* @property {clearInterval} clearInterval
Expand Down Expand Up @@ -1789,6 +1795,8 @@
return uninstall(clock, config);
};

clock.abortListenerMap = new Map();
fatso83 marked this conversation as resolved.
Show resolved Hide resolved

clock.methods = config.toFake || [];

if (clock.methods.length === 0) {
Expand Down Expand Up @@ -1895,6 +1903,8 @@
"abort",
abort,
);
clock.abortListenerMap.delete(abort);

// This is safe, there is no code path that leads to this function
// being invoked before handle has been assigned.
// eslint-disable-next-line no-use-before-define
Expand All @@ -1903,21 +1913,30 @@
};

const handle = clock.setTimeout(() => {
options.signal?.removeEventListener(
"abort",
abort,
);
if (options.signal) {
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
options.signal.removeEventListener(
"abort",
abort,
);
clock.abortListenerMap.delete(abort);
}

resolve(value);
}, delay);

if (options.signal?.aborted) {
abort();
} else {
options.signal?.addEventListener(
"abort",
abort,
);
if (options.signal) {
if (options.signal.aborted) {
abort();
} else {
options.signal.addEventListener(
"abort",
abort,
);
clock.abortListenerMap.set(
abort,
options.signal,
);
}
}
});
} else if (nameOfMethodToReplace === "setImmediate") {
Expand All @@ -1933,6 +1952,8 @@
"abort",
abort,
);
clock.abortListenerMap.delete(abort);

// This is safe, there is no code path that leads to this function
// being invoked before handle has been assigned.
// eslint-disable-next-line no-use-before-define
Expand All @@ -1941,21 +1962,30 @@
};

const handle = clock.setImmediate(() => {
options.signal?.removeEventListener(
"abort",
abort,
);
if (options.signal) {
options.signal.removeEventListener(
"abort",
abort,
);
clock.abortListenerMap.delete(abort);
}

resolve(value);
});

if (options.signal?.aborted) {
abort();
} else {
options.signal?.addEventListener(
"abort",
abort,
);
if (options.signal) {
if (options.signal.aborted) {
abort();
} else {
options.signal.addEventListener(
"abort",
abort,
);
clock.abortListenerMap.set(
abort,
options.signal,
);
}
}
});
} else if (nameOfMethodToReplace === "setInterval") {
Expand Down Expand Up @@ -2000,20 +2030,28 @@
"abort",
abort,
);
clock.abortListenerMap.delete(abort);

clock.clearInterval(handle);
done = true;
for (const resolvable of nextQueue) {
resolvable.resolve();
}
};

if (options.signal?.aborted) {
done = true;
} else {
options.signal?.addEventListener(
"abort",
abort,
);
if (options.signal) {
if (options.signal.aborted) {
done = true;
} else {
options.signal.addEventListener(
"abort",
abort,
);
clock.abortListenerMap.set(
abort,
options.signal,
);
}
}

return {
Expand Down Expand Up @@ -2065,10 +2103,13 @@
clock.clearInterval(handle);
done = true;

options.signal?.removeEventListener(
"abort",
abort,
);
if (options.signal) {
options.signal.removeEventListener(
"abort",
abort,
);
clock.abortListenerMap.delete(abort);
}

return { done: true, value: undefined };
},
Expand Down
134 changes: 134 additions & 0 deletions test/fake-timers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5068,6 +5068,49 @@ describe("FakeTimers", function () {
true,
);
});

it("should remove abort listener when uninstalling", function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
timersPromisesModule.setTimeout(100, null, {
signal: abortController.signal,
});

clock.uninstall();
clock = undefined;

assert.equals(
abortController.signal.removeEventListener.called,
true,
);
});

it("should remove listener from abort listener map when aborting", async function () {
clock = FakeTimers.install();
const abortController = new AbortController();
const promise = timersPromisesModule.setTimeout(100, null, {
signal: abortController.signal,
});

abortController.abort();
await promise.catch(() => {});

assert.equals(clock.abortListenerMap.size, 0);
});

it("should remove listener from abort listener map when resolving", async function () {
clock = FakeTimers.install();
const abortController = new AbortController();
const promise = timersPromisesModule.setTimeout(100, null, {
signal: abortController.signal,
});

clock.tick(100);
await promise;

assert.equals(clock.abortListenerMap.size, 0);
});
});

describe("The setImmediate function", function () {
Expand Down Expand Up @@ -5140,6 +5183,51 @@ describe("FakeTimers", function () {
true,
);
});

it("should remove abort listener when uninstalling", function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
timersPromisesModule.setImmediate(null, {
signal: abortController.signal,
});

clock.uninstall();
clock = undefined;

assert.equals(
abortController.signal.removeEventListener.called,
true,
);
});

it("should remove listener from abort listener map when aborting", async function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
const promise = timersPromisesModule.setImmediate(null, {
signal: abortController.signal,
});

abortController.abort();
await promise.catch(() => {});

assert.equals(clock.abortListenerMap.size, 0);
});

it("should remove listener from abort listener map when resolving", async function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
const promise = timersPromisesModule.setImmediate(null, {
signal: abortController.signal,
});

clock.tick(0);
await promise;

assert.equals(clock.abortListenerMap.size, 0);
});
});

describe("The setInterval function", function () {
Expand Down Expand Up @@ -5389,6 +5477,52 @@ describe("FakeTimers", function () {
true,
);
});

it("should remove abort listener when uninstalling", function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
const iterable = timersPromisesModule.setInterval(100, null, {
signal: abortController.signal,
});
iterable[Symbol.asyncIterator]();

clock.uninstall();
clock = undefined;

assert.equals(
abortController.signal.removeEventListener.called,
true,
);
});

it("should remove listener from abort listener map when aborting", function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
const iterable = timersPromisesModule.setInterval(100, null, {
signal: abortController.signal,
});
iterable[Symbol.asyncIterator]();

abortController.abort();

assert.equals(clock.abortListenerMap.size, 0);
});

it("should remove listener from abort listener map when returning", async function () {
clock = FakeTimers.install();
const abortController = new AbortController();
abortController.signal.removeEventListener = sinon.stub();
const iterable = timersPromisesModule.setInterval(100, null, {
signal: abortController.signal,
});
const iter = iterable[Symbol.asyncIterator]();

await iter.return();

assert.equals(clock.abortListenerMap.size, 0);
});
});
});
});
Expand Down
Loading