Skip to content

Commit 982f1b1

Browse files
alan-agius4atscott
alan-agius4
authored and
atscott
committed
fix(zone.js): support Timeout.refresh in Node.js (#56852)
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: #56586 PR Close #56852
1 parent 2c4613a commit 982f1b1

6 files changed

Lines changed: 164 additions & 64 deletions

File tree

‎packages/zone.js/lib/common/timers.ts‎

Lines changed: 67 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,15 @@
1010
* @suppress {missingRequire}
1111
*/
1212

13-
import {patchMethod, scheduleMacroTaskWithCurrentZone, zoneSymbol} from './utils';
13+
import {
14+
isFunction,
15+
isNumber,
16+
patchMethod,
17+
scheduleMacroTaskWithCurrentZone,
18+
zoneSymbol,
19+
} from './utils';
1420

15-
const taskSymbol = zoneSymbol('zoneTask');
21+
export const taskSymbol = zoneSymbol('zoneTask');
1622

1723
interface TimerOptions extends TaskData {
1824
handleId?: number;
@@ -32,25 +38,42 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
3238
data.args[0] = function () {
3339
return task.invoke.apply(this, arguments);
3440
};
35-
data.handleId = setNative!.apply(window, data.args);
41+
42+
const handleOrId = setNative!.apply(window, data.args);
43+
44+
// Whlist on Node.js when get can the ID by using `[Symbol.toPrimitive]()` we do
45+
// to this so that we do not cause potentally leaks when using `setTimeout`
46+
// since this can be periodic when using `.refresh`.
47+
if (isNumber(handleOrId)) {
48+
data.handleId = handleOrId;
49+
} else {
50+
data.handle = handleOrId;
51+
// On Node.js a timeout and interval can be restarted over and over again by using the `.refresh` method.
52+
data.isRefreshable = isFunction(handleOrId.refresh);
53+
}
54+
3655
return task;
3756
}
3857

3958
function clearTask(task: Task) {
40-
return clearNative!.call(window, (<TimerOptions>task.data).handleId);
59+
const {handle, handleId} = task.data!;
60+
61+
return clearNative!.call(window, handle ?? handleId);
4162
}
4263

4364
setNative = patchMethod(
4465
window,
4566
setName,
4667
(delegate: Function) =>
4768
function (self: any, args: any[]) {
48-
if (typeof args[0] === 'function') {
69+
if (isFunction(args[0])) {
4970
const options: TimerOptions = {
71+
isRefreshable: false,
5072
isPeriodic: nameSuffix === 'Interval',
5173
delay: nameSuffix === 'Timeout' || nameSuffix === 'Interval' ? args[1] || 0 : undefined,
5274
args: args,
5375
};
76+
5477
const callback = args[0];
5578
args[0] = function timer(this: unknown) {
5679
try {
@@ -64,15 +87,17 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
6487
// Cleanup tasksByHandleId should be handled before scheduleTask
6588
// Since some zoneSpec may intercept and doesn't trigger
6689
// scheduleFn(scheduleTask) provided here.
67-
if (!options.isPeriodic) {
68-
if (typeof options.handleId === 'number') {
90+
const {handle, handleId, isPeriodic, isRefreshable} = options;
91+
92+
if (!isPeriodic && !isRefreshable) {
93+
if (handleId) {
6994
// in non-nodejs env, we remove timerId
7095
// from local cache
71-
delete tasksByHandleId[options.handleId];
72-
} else if (options.handleId) {
96+
delete tasksByHandleId[handleId];
97+
} else if (handle) {
7398
// Node returns complex objects as handleIds
7499
// we remove task reference from timer object
75-
(options.handleId as any)[taskSymbol] = null;
100+
handle[taskSymbol] = null;
76101
}
77102
}
78103
}
@@ -84,37 +109,39 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
84109
scheduleTask,
85110
clearTask,
86111
);
112+
87113
if (!task) {
88114
return task;
89115
}
116+
90117
// Node.js must additionally support the ref and unref functions.
91-
const handle: any = (<TimerOptions>task.data).handleId;
92-
if (typeof handle === 'number') {
118+
const {handleId, handle, isRefreshable, isPeriodic} = <TimerOptions>task.data;
119+
if (handleId) {
93120
// for non nodejs env, we save handleId: task
94121
// mapping in local cache for clearTimeout
95-
tasksByHandleId[handle] = task;
122+
tasksByHandleId[handleId] = task;
96123
} else if (handle) {
97124
// for nodejs env, we save task
98125
// reference in timerId Object for clearTimeout
99126
handle[taskSymbol] = task;
100-
}
101127

102-
// check whether handle is null, because some polyfill or browser
103-
// may return undefined from setTimeout/setInterval/setImmediate/requestAnimationFrame
104-
if (
105-
handle &&
106-
handle.ref &&
107-
handle.unref &&
108-
typeof handle.ref === 'function' &&
109-
typeof handle.unref === 'function'
110-
) {
111-
(<any>task).ref = (<any>handle).ref.bind(handle);
112-
(<any>task).unref = (<any>handle).unref.bind(handle);
113-
}
114-
if (typeof handle === 'number' || handle) {
115-
return handle;
128+
if (isRefreshable && !isPeriodic) {
129+
const originalRefresh = handle.refresh;
130+
handle.refresh = function () {
131+
const {zone, state} = task as any;
132+
if (state === 'notScheduled') {
133+
(task as any)._state = 'scheduled';
134+
zone._updateTaskCount(task, 1);
135+
} else if (state === 'running') {
136+
(task as any)._state = 'scheduling';
137+
}
138+
139+
return originalRefresh.call(this);
140+
};
141+
}
116142
}
117-
return task;
143+
144+
return handle ?? handleId ?? task;
118145
} else {
119146
// cause an error by calling it directly.
120147
return delegate.apply(window, args);
@@ -129,27 +156,23 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
129156
function (self: any, args: any[]) {
130157
const id = args[0];
131158
let task: Task;
132-
if (typeof id === 'number') {
159+
160+
if (isNumber(id)) {
133161
// non nodejs env.
134162
task = tasksByHandleId[id];
163+
delete tasksByHandleId[id];
135164
} else {
136-
// nodejs env.
137-
task = id && id[taskSymbol];
138-
// other environments.
139-
if (!task) {
165+
// nodejs env ?? other environments.
166+
task = id?.[taskSymbol];
167+
if (task) {
168+
id[taskSymbol] = null;
169+
} else {
140170
task = id;
141171
}
142172
}
143-
if (task && typeof task.type === 'string') {
144-
if (
145-
task.state !== 'notScheduled' &&
146-
((task.cancelFn && task.data!.isPeriodic) || task.runCount === 0)
147-
) {
148-
if (typeof id === 'number') {
149-
delete tasksByHandleId[id];
150-
} else if (id) {
151-
id[taskSymbol] = null;
152-
}
173+
174+
if (task?.type) {
175+
if (task.cancelFn) {
153176
// Do not cancel already canceled functions
154177
task.zone.cancelTask(task);
155178
}

‎packages/zone.js/lib/common/utils.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,3 +567,11 @@ export function isIEOrEdge() {
567567
} catch (error) {}
568568
return ieOrEdge;
569569
}
570+
571+
export function isFunction(value: unknown): value is Function {
572+
return typeof value === 'function';
573+
}
574+
575+
export function isNumber(value: unknown): value is number {
576+
return typeof value === 'number';
577+
}

‎packages/zone.js/lib/zone-impl.ts‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,11 @@ export declare interface TaskData {
644644
*/
645645
isPeriodic?: boolean;
646646

647+
/**
648+
* A [MacroTask] that can be manually rescheduled.
649+
*/
650+
isRefreshable?: boolean;
651+
647652
/**
648653
* Delay in milliseconds when the Task will run.
649654
*/
@@ -653,6 +658,9 @@ export declare interface TaskData {
653658
* identifier returned by the native setTimeout.
654659
*/
655660
handleId?: number;
661+
662+
/** The target handler. */
663+
handle?: any;
656664
}
657665

658666
/**
@@ -925,26 +933,29 @@ export function initZone(): ZoneType {
925933
')',
926934
);
927935
}
936+
937+
const zoneTask = task as ZoneTask<any>;
928938
// https://github.com/angular/zone.js/issues/778, sometimes eventTask
929939
// will run in notScheduled(canceled) state, we should not try to
930940
// run such kind of task but just return
941+
const {type, data: {isPeriodic = false, isRefreshable = false} = {}} = task;
931942

932-
if (task.state === notScheduled && (task.type === eventTask || task.type === macroTask)) {
943+
if (task.state === notScheduled && (type === eventTask || type === macroTask)) {
933944
return;
934945
}
935946

936947
const reEntryGuard = task.state != running;
937-
reEntryGuard && (task as ZoneTask<any>)._transitionTo(running, scheduled);
938-
task.runCount++;
948+
reEntryGuard && zoneTask._transitionTo(running, scheduled);
939949
const previousTask = _currentTask;
940-
_currentTask = task;
950+
_currentTask = zoneTask;
941951
_currentZoneFrame = {parent: _currentZoneFrame, zone: this};
952+
942953
try {
943-
if (task.type == macroTask && task.data && !task.data.isPeriodic) {
954+
if (type == macroTask && task.data && !isPeriodic && !isRefreshable) {
944955
task.cancelFn = undefined;
945956
}
946957
try {
947-
return this._zoneDelegate.invokeTask(this, task, applyThis, applyArgs);
958+
return this._zoneDelegate.invokeTask(this, zoneTask, applyThis, applyArgs);
948959
} catch (error) {
949960
if (this._zoneDelegate.handleError(this, error)) {
950961
throw error;
@@ -953,14 +964,18 @@ export function initZone(): ZoneType {
953964
} finally {
954965
// if the task's state is notScheduled or unknown, then it has already been cancelled
955966
// we should not reset the state to scheduled
956-
if (task.state !== notScheduled && task.state !== unknown) {
957-
if (task.type == eventTask || (task.data && task.data.isPeriodic)) {
958-
reEntryGuard && (task as ZoneTask<any>)._transitionTo(scheduled, running);
967+
const state = task.state;
968+
if (state !== notScheduled && state !== unknown) {
969+
if (type == eventTask || isPeriodic || (isRefreshable && state === scheduling)) {
970+
reEntryGuard && zoneTask._transitionTo(scheduled, running, scheduling);
959971
} else {
960-
task.runCount = 0;
961-
this._updateTaskCount(task as ZoneTask<any>, -1);
962-
reEntryGuard &&
963-
(task as ZoneTask<any>)._transitionTo(notScheduled, running, notScheduled);
972+
const zoneDelegates = zoneTask._zoneDelegates;
973+
this._updateTaskCount(zoneTask, -1);
974+
reEntryGuard && zoneTask._transitionTo(notScheduled, running, notScheduled);
975+
976+
if (isRefreshable) {
977+
zoneTask._zoneDelegates = zoneDelegates;
978+
}
964979
}
965980
}
966981
_currentZoneFrame = _currentZoneFrame.parent!;
@@ -1066,7 +1081,7 @@ export function initZone(): ZoneType {
10661081
}
10671082
this._updateTaskCount(task as ZoneTask<any>, -1);
10681083
(task as ZoneTask<any>)._transitionTo(notScheduled, canceling);
1069-
task.runCount = 0;
1084+
task.runCount = -1;
10701085
return task;
10711086
}
10721087

‎packages/zone.js/test/node/timer.spec.ts‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.io/license
77
*/
8-
import {promisify} from 'util';
8+
import {setTimeout as sleep} from 'node:timers/promises';
9+
import {promisify} from 'node:util';
10+
import {taskSymbol} from '../../lib/common/timers';
911

1012
describe('node timer', () => {
1113
it('util.promisify should work with setTimeout', (done: DoneFn) => {
@@ -33,4 +35,60 @@ describe('node timer', () => {
3335
},
3436
);
3537
});
38+
39+
it(`'Timeout.refresh' should restart the 'setTimeout' when it is not scheduled`, async () => {
40+
const spy = jasmine.createSpy();
41+
const timeout = setTimeout(spy, 100) as unknown as NodeJS.Timeout;
42+
43+
let iterations = 5;
44+
for (let i = 1; i <= iterations; i++) {
45+
timeout.refresh();
46+
await sleep(150);
47+
}
48+
49+
expect((timeout as any)[taskSymbol].runCount).toBe(iterations);
50+
51+
clearTimeout(timeout);
52+
53+
expect((timeout as any)[taskSymbol]).toBeNull();
54+
expect(spy).toHaveBeenCalledTimes(iterations);
55+
});
56+
57+
it(`'Timeout.refresh' restarts the 'setTimeout' when it is running`, async () => {
58+
let timeout: NodeJS.Timeout;
59+
const spy = jasmine.createSpy().and.callFake(() => timeout.refresh());
60+
timeout = setTimeout(spy, 100) as unknown as NodeJS.Timeout;
61+
62+
await sleep(250);
63+
64+
expect((timeout as any)[taskSymbol].runCount).toBe(2);
65+
66+
clearTimeout(timeout);
67+
68+
expect((timeout as any)[taskSymbol]).toBeNull();
69+
expect(spy).toHaveBeenCalledTimes(2);
70+
});
71+
72+
it(`'Timeout.refresh' should restart the 'setInterval'`, async () => {
73+
const spy = jasmine.createSpy();
74+
const interval = setInterval(spy, 200) as unknown as NodeJS.Timeout;
75+
76+
// Restart the interval multiple times before the elapsed time.
77+
for (let i = 1; i <= 4; i++) {
78+
interval.refresh();
79+
await sleep(100);
80+
}
81+
82+
// Time did not elapse
83+
expect((interval as any)[taskSymbol].runCount).toBe(0);
84+
expect(spy).toHaveBeenCalledTimes(0);
85+
86+
await sleep(350);
87+
expect((interval as any)[taskSymbol].runCount).toBe(2);
88+
89+
clearInterval(interval);
90+
91+
expect((interval as any)[taskSymbol]).toBeNull();
92+
expect(spy).toHaveBeenCalledTimes(2);
93+
});
3694
});

‎packages/zone.js/test/zone-spec/async-test.spec.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,6 @@ describe('AsyncTestZoneSpec', function () {
1313
let log: string[];
1414
const AsyncTestZoneSpec = (Zone as any)['AsyncTestZoneSpec'];
1515

16-
function finishCallback() {
17-
log.push('finish');
18-
}
19-
2016
function failCallback() {
2117
log.push('fail');
2218
}

‎packages/zone.js/test/zone-spec/fake-async-test.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,7 @@ describe('FakeAsyncTestZoneSpec', () => {
324324
// will call `task.state` one more time to check whether to clear the
325325
// task or not, so here we use this count to check the issue is fixed or not
326326
// For details, please refer to https://github.com/angular/angular/issues/40387
327-
expect(taskSpy.calls.count()).toEqual(5);
327+
expect(taskSpy.calls.count()).toBe(4);
328328
});
329329
});
330330
it(

0 commit comments

Comments
 (0)