fix(zone.js): setTimeout patch should clean tasksByHandleId cache #40586
Conversation
a00971d to
29e0a7a
Compare
|
@mhevery , thanks for the review, I have updated the commit message, but since this |
2faa5ad to
098bf71
Compare
I don't think you need to have |
|
@mhevery , yeah, I understand your point, now most cases behave correctly even the |
Close angular#40387 Currently zone.js patches `setTimeout` and keeps a `tasksByHandleId` map to keep `timerId` <-> `ZoneTask` relationship. This is needed so that when `clearTimeout(timerId)` is called, zone.js can find the associated `ZoneTask`. Now zone.js set the `tasksByHandleId` map in the `scheduleTask` function, but if the `setTimeout` is running in the `FakeAsyncZoneSpec` or any other `ZoneSpec` with `onScheduleTask` hooks. The `scheduleTask` in `timer` patch may not be invoked. For example: ``` fakeAsync(() => { setTimeout(() => {}); tick(); }); ``` In this case, the `timerId` kept in the `tasksByHandleId` map is not cleared. This is because the `FakeAsyncZoneSpec` in the `onScheduleTask` hook looks like this. ``` onScheduleTask(delegate, ..., task) { fakeAsyncScheduler.setTimeout(task); return task; } ``` Because `FakeAsyncZoneSpec` handles the task itself and it doesn't call `parentDelegate.onScheduleTask`, therefore the default `scheduleTask` in the `timer` patch is not invoked. In this commit, the cleanup logic is moved from `scheduleTask` to `setTimeout` patch entry to avoid the memory leak.
098bf71 to
be04789
Compare
|
@mhevery , I have added one test case to describe the fix, so this issue will only cause memory leak when using |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Close #40387
Currently zone.js patches
setTimeoutand keeps atasksByHandleIdmap to keeptimerId<->ZoneTaskrelationship. This is needed so that whenclearTimeout(timerId)is called, zone.js can find the associatedZoneTask. Now zone.js set thetasksByHandleIdmap in thescheduleTaskfunction, but if thesetTimeoutis running in theFakeAsyncZoneSpecor any otherZoneSpecwithonScheduleTaskhooks. ThescheduleTaskintimerpatch may not be invoked.For example:
In this case, the
timerIdkept in thetasksByHandleIdmap is not cleared.This is because the
FakeAsyncZoneSpecin theonScheduleTaskhook looks like this.Because
FakeAsyncZoneSpechandles the task itself and it doesn't callparentDelegate.onScheduleTask,therefore the default
scheduleTaskin thetimerpatch is not invoked.In this commit, the cleanup logic is moved from
scheduleTasktosetTimeoutpatch entry toavoid the memory leak.