Skip to content

Commit

Permalink
fix(zone.js): support Timeout.refresh in Node.js (angular#56852)
Browse files Browse the repository at this point in the history 
The `Timeout` object in Node.js has a `refresh` method, used to restart `setTimeout`/`setInterval` timers. Before this commit, `Timeout.refresh` was not handled, leading to memory leaks when using `fetch` in Node.js. This issue arose because `undici` (the Node.js fetch implementation) uses a refreshed `setTimeout` for cleanup operations.

For reference, see: https://github.com/nodejs/undici/blob/1dff4fd9b1b2cee97c5f8cf44041521a62d3f133/lib/util/timers.js#L45

Fixes: angular#56586

PR Close angular#56852
  • Loading branch information
@alan-agius4 @atscott
alan-agius4 authored and atscott committed Jul 16, 2024
1 parent 2c4613a commit 982f1b1
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 64 deletions.
111 changes: 67 additions & 44 deletions packages/zone.js/lib/common/timers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,15 @@
* @suppress {missingRequire}
*/

import {patchMethod, scheduleMacroTaskWithCurrentZone, zoneSymbol} from './utils';
import {
isFunction,
isNumber,
patchMethod,
scheduleMacroTaskWithCurrentZone,
zoneSymbol,
} from './utils';

const taskSymbol = zoneSymbol('zoneTask');
export const taskSymbol = zoneSymbol('zoneTask');

interface TimerOptions extends TaskData {
handleId?: number;
Expand All @@ -32,25 +38,42 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
data.args[0] = function () {
return task.invoke.apply(this, arguments);
};
data.handleId = setNative!.apply(window, data.args);

const handleOrId = setNative!.apply(window, data.args);

// Whlist on Node.js when get can the ID by using `[Symbol.toPrimitive]()` we do
// to this so that we do not cause potentally leaks when using `setTimeout`
// since this can be periodic when using `.refresh`.
if (isNumber(handleOrId)) {
data.handleId = handleOrId;
} else {
data.handle = handleOrId;
// On Node.js a timeout and interval can be restarted over and over again by using the `.refresh` method.
data.isRefreshable = isFunction(handleOrId.refresh);
}

return task;
}

function clearTask(task: Task) {
return clearNative!.call(window, (<TimerOptions>task.data).handleId);
const {handle, handleId} = task.data!;

return clearNative!.call(window, handle ?? handleId);
}

setNative = patchMethod(
window,
setName,
(delegate: Function) =>
function (self: any, args: any[]) {
if (typeof args[0] === 'function') {
if (isFunction(args[0])) {
const options: TimerOptions = {
isRefreshable: false,
isPeriodic: nameSuffix === 'Interval',
delay: nameSuffix === 'Timeout' || nameSuffix === 'Interval' ? args[1] || 0 : undefined,
args: args,
};

const callback = args[0];
args[0] = function timer(this: unknown) {
try {
Expand All @@ -64,15 +87,17 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
// Cleanup tasksByHandleId should be handled before scheduleTask
// Since some zoneSpec may intercept and doesn't trigger
// scheduleFn(scheduleTask) provided here.
if (!options.isPeriodic) {
if (typeof options.handleId === 'number') {
const {handle, handleId, isPeriodic, isRefreshable} = options;

if (!isPeriodic && !isRefreshable) {
if (handleId) {
// in non-nodejs env, we remove timerId
// from local cache
delete tasksByHandleId[options.handleId];
} else if (options.handleId) {
delete tasksByHandleId[handleId];
} else if (handle) {
// Node returns complex objects as handleIds
// we remove task reference from timer object
(options.handleId as any)[taskSymbol] = null;
handle[taskSymbol] = null;
}
}
}
Expand All @@ -84,37 +109,39 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
scheduleTask,
clearTask,
);

if (!task) {
return task;
}

// Node.js must additionally support the ref and unref functions.
const handle: any = (<TimerOptions>task.data).handleId;
if (typeof handle === 'number') {
const {handleId, handle, isRefreshable, isPeriodic} = <TimerOptions>task.data;
if (handleId) {
// for non nodejs env, we save handleId: task
// mapping in local cache for clearTimeout
tasksByHandleId[handle] = task;
tasksByHandleId[handleId] = task;
} else if (handle) {
// for nodejs env, we save task
// reference in timerId Object for clearTimeout
handle[taskSymbol] = task;
}

// check whether handle is null, because some polyfill or browser
// may return undefined from setTimeout/setInterval/setImmediate/requestAnimationFrame
if (
handle &&
handle.ref &&
handle.unref &&
typeof handle.ref === 'function' &&
typeof handle.unref === 'function'
) {
(<any>task).ref = (<any>handle).ref.bind(handle);
(<any>task).unref = (<any>handle).unref.bind(handle);
}
if (typeof handle === 'number' || handle) {
return handle;
if (isRefreshable && !isPeriodic) {
const originalRefresh = handle.refresh;
handle.refresh = function () {
const {zone, state} = task as any;
if (state === 'notScheduled') {
(task as any)._state = 'scheduled';
zone._updateTaskCount(task, 1);
} else if (state === 'running') {
(task as any)._state = 'scheduling';
}

return originalRefresh.call(this);
};
}
}
return task;

return handle ?? handleId ?? task;
} else {
// cause an error by calling it directly.
return delegate.apply(window, args);
Expand All @@ -129,27 +156,23 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
function (self: any, args: any[]) {
const id = args[0];
let task: Task;
if (typeof id === 'number') {

if (isNumber(id)) {
// non nodejs env.
task = tasksByHandleId[id];
delete tasksByHandleId[id];
} else {
// nodejs env.
task = id && id[taskSymbol];
// other environments.
if (!task) {
// nodejs env ?? other environments.
task = id?.[taskSymbol];
if (task) {
id[taskSymbol] = null;
} else {
task = id;
}
}
if (task && typeof task.type === 'string') {
if (
task.state !== 'notScheduled' &&
((task.cancelFn && task.data!.isPeriodic) || task.runCount === 0)
) {
if (typeof id === 'number') {
delete tasksByHandleId[id];
} else if (id) {
id[taskSymbol] = null;
}

if (task?.type) {
if (task.cancelFn) {
// Do not cancel already canceled functions
task.zone.cancelTask(task);
}
Expand Down
8 changes: 8 additions & 0 deletions packages/zone.js/lib/common/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,3 +567,11 @@ export function isIEOrEdge() {
} catch (error) {}
return ieOrEdge;
}

export function isFunction(value: unknown): value is Function {
return typeof value === 'function';
}

export function isNumber(value: unknown): value is number {
return typeof value === 'number';
}
43 changes: 29 additions & 14 deletions packages/zone.js/lib/zone-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,11 @@ export declare interface TaskData {
*/
isPeriodic?: boolean;

/**
* A [MacroTask] that can be manually rescheduled.
*/
isRefreshable?: boolean;

/**
* Delay in milliseconds when the Task will run.
*/
Expand All @@ -653,6 +658,9 @@ export declare interface TaskData {
* identifier returned by the native setTimeout.
*/
handleId?: number;

/** The target handler. */
handle?: any;
}

/**
Expand Down Expand Up @@ -925,26 +933,29 @@ export function initZone(): ZoneType {
')',
);
}

const zoneTask = task as ZoneTask<any>;
// https://github.com/angular/zone.js/issues/778, sometimes eventTask
// will run in notScheduled(canceled) state, we should not try to
// run such kind of task but just return
const {type, data: {isPeriodic = false, isRefreshable = false} = {}} = task;

if (task.state === notScheduled && (task.type === eventTask || task.type === macroTask)) {
if (task.state === notScheduled && (type === eventTask || type === macroTask)) {
return;
}

const reEntryGuard = task.state != running;
reEntryGuard && (task as ZoneTask<any>)._transitionTo(running, scheduled);
task.runCount++;
reEntryGuard && zoneTask._transitionTo(running, scheduled);
const previousTask = _currentTask;
_currentTask = task;
_currentTask = zoneTask;
_currentZoneFrame = {parent: _currentZoneFrame, zone: this};

try {
if (task.type == macroTask && task.data && !task.data.isPeriodic) {
if (type == macroTask && task.data && !isPeriodic && !isRefreshable) {
task.cancelFn = undefined;
}
try {
return this._zoneDelegate.invokeTask(this, task, applyThis, applyArgs);
return this._zoneDelegate.invokeTask(this, zoneTask, applyThis, applyArgs);
} catch (error) {
if (this._zoneDelegate.handleError(this, error)) {
throw error;
Expand All @@ -953,14 +964,18 @@ export function initZone(): ZoneType {
} finally {
// if the task's state is notScheduled or unknown, then it has already been cancelled
// we should not reset the state to scheduled
if (task.state !== notScheduled && task.state !== unknown) {
if (task.type == eventTask || (task.data && task.data.isPeriodic)) {
reEntryGuard && (task as ZoneTask<any>)._transitionTo(scheduled, running);
const state = task.state;
if (state !== notScheduled && state !== unknown) {
if (type == eventTask || isPeriodic || (isRefreshable && state === scheduling)) {
reEntryGuard && zoneTask._transitionTo(scheduled, running, scheduling);
} else {
task.runCount = 0;
this._updateTaskCount(task as ZoneTask<any>, -1);
reEntryGuard &&
(task as ZoneTask<any>)._transitionTo(notScheduled, running, notScheduled);
const zoneDelegates = zoneTask._zoneDelegates;
this._updateTaskCount(zoneTask, -1);
reEntryGuard && zoneTask._transitionTo(notScheduled, running, notScheduled);

if (isRefreshable) {
zoneTask._zoneDelegates = zoneDelegates;
}
}
}
_currentZoneFrame = _currentZoneFrame.parent!;
Expand Down Expand Up @@ -1066,7 +1081,7 @@ export function initZone(): ZoneType {
}
this._updateTaskCount(task as ZoneTask<any>, -1);
(task as ZoneTask<any>)._transitionTo(notScheduled, canceling);
task.runCount = 0;
task.runCount = -1;
return task;
}

Expand Down
60 changes: 59 additions & 1 deletion packages/zone.js/test/node/timer.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {promisify} from 'util';
import {setTimeout as sleep} from 'node:timers/promises';
import {promisify} from 'node:util';
import {taskSymbol} from '../../lib/common/timers';

describe('node timer', () => {
it('util.promisify should work with setTimeout', (done: DoneFn) => {
Expand Down Expand Up @@ -33,4 +35,60 @@ describe('node timer', () => {
},
);
});

it(`'Timeout.refresh' should restart the 'setTimeout' when it is not scheduled`, async () => {
const spy = jasmine.createSpy();
const timeout = setTimeout(spy, 100) as unknown as NodeJS.Timeout;

let iterations = 5;
for (let i = 1; i <= iterations; i++) {
timeout.refresh();
await sleep(150);
}

expect((timeout as any)[taskSymbol].runCount).toBe(iterations);

clearTimeout(timeout);

expect((timeout as any)[taskSymbol]).toBeNull();
expect(spy).toHaveBeenCalledTimes(iterations);
});

it(`'Timeout.refresh' restarts the 'setTimeout' when it is running`, async () => {
let timeout: NodeJS.Timeout;
const spy = jasmine.createSpy().and.callFake(() => timeout.refresh());
timeout = setTimeout(spy, 100) as unknown as NodeJS.Timeout;

await sleep(250);

expect((timeout as any)[taskSymbol].runCount).toBe(2);

clearTimeout(timeout);

expect((timeout as any)[taskSymbol]).toBeNull();
expect(spy).toHaveBeenCalledTimes(2);
});

it(`'Timeout.refresh' should restart the 'setInterval'`, async () => {
const spy = jasmine.createSpy();
const interval = setInterval(spy, 200) as unknown as NodeJS.Timeout;

// Restart the interval multiple times before the elapsed time.
for (let i = 1; i <= 4; i++) {
interval.refresh();
await sleep(100);
}

// Time did not elapse
expect((interval as any)[taskSymbol].runCount).toBe(0);
expect(spy).toHaveBeenCalledTimes(0);

await sleep(350);
expect((interval as any)[taskSymbol].runCount).toBe(2);

clearInterval(interval);

expect((interval as any)[taskSymbol]).toBeNull();
expect(spy).toHaveBeenCalledTimes(2);
});
});
4 changes: 0 additions & 4 deletions packages/zone.js/test/zone-spec/async-test.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ describe('AsyncTestZoneSpec', function () {
let log: string[];
const AsyncTestZoneSpec = (Zone as any)['AsyncTestZoneSpec'];

function finishCallback() {
log.push('finish');
}

function failCallback() {
log.push('fail');
}
Expand Down
2 changes: 1 addition & 1 deletion packages/zone.js/test/zone-spec/fake-async-test.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ describe('FakeAsyncTestZoneSpec', () => {
// will call `task.state` one more time to check whether to clear the
// task or not, so here we use this count to check the issue is fixed or not
// For details, please refer to https://github.com/angular/angular/issues/40387
expect(taskSpy.calls.count()).toEqual(5);
expect(taskSpy.calls.count()).toBe(4);
});
});
it(
Expand Down

0 comments on commit 982f1b1

Please sign in to comment.