Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix(core): don't update taskCounts of setInterval #999

Closed
wants to merge 1 commit into from
Closed

fix(core): don't update taskCounts of setInterval #999

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
7 changes: 5 additions & 2 deletions lib/zone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,8 +846,9 @@ const Zone: ZoneType = (function(global: any) {
this._zoneDelegate.handleError(this, err);
throw err;
}
if ((task as any as ZoneTask<any>)._zoneDelegates === zoneDelegates) {
if ((task as any as ZoneTask<any>)._zoneDelegates === zoneDelegates && !(task.data && task.data.isPeriodic)) {
// we have to check because internally the delegate can reschedule the task.
// and we don't need to updateTaskCount for periodic task such as setInterval
this._updateTaskCount(task as any as ZoneTask<any>, 1);
}
if ((task as any as ZoneTask<any>).state == scheduling) {
Expand Down Expand Up @@ -891,7 +892,9 @@ const Zone: ZoneType = (function(global: any) {
this._zoneDelegate.handleError(this, err);
throw err;
}
this._updateTaskCount(task as ZoneTask<any>, -1);
if (!(task.data && task.data.isPeriodic)) {
this._updateTaskCount(task as ZoneTask<any>, -1);
}
(task as ZoneTask<any>)._transitionTo(notScheduled, canceling);
task.runCount = 0;
return task;
Expand Down
19 changes: 9 additions & 10 deletions test/common/setInterval.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,15 @@ describe('setInterval', function() {
}, null, null, 'unit-test');
});

it('should not cancel the task after invoke the setInterval callback', (done) => {
it('setInterval should not update task count to trigger onHasTask', (done: Function) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. Why should setInterval be exempt from tasks?

Copy link
Collaborator Author

@JiaLiPassion JiaLiPassion Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhevery
I just re-think it, you are right, we should not exempt interval from tasks, the interval should behave like

  • call updateTaskCount +1 and trigger onHasTask when scheduleTask.
  • will ** not ** call updateTaskCount and trigger onHasTask after runTask.
  • will call updateTaskCount -1 and trigger onHasTask when cancelTask.

so if some interval task is active, zone will not be stable, but it is the correct behavior.

const logs: HasTaskState[] = [];
let task: Task;
const zone = Zone.current.fork({
name: 'interval',
onScheduleTask: (delegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, t: Task) => {
task = t;
return delegate.scheduleTask(targetZone, task);
},
onHasTask:
(delegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, hasTask: HasTaskState) => {
logs.push(hasTask);
Expand All @@ -74,17 +79,11 @@ describe('setInterval', function() {
zone.run(() => {
const timerId = setInterval(() => {}, 100);
(global as any)[Zone.__symbol__('setTimeout')](() => {
expect(logs.length > 0).toBeTruthy();
expect(logs).toEqual(
[{microTask: false, macroTask: true, eventTask: false, change: 'macroTask'}]);
expect(logs).toEqual([]);
clearInterval(timerId);
expect(logs).toEqual([
{microTask: false, macroTask: true, eventTask: false, change: 'macroTask'},
{microTask: false, macroTask: false, eventTask: false, change: 'macroTask'}
]);
expect(logs).toEqual([]);
done();
}, 300);
});
});
});

});
9 changes: 1 addition & 8 deletions test/common/zone.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,7 @@ describe('Zone', function() {
zone.cancelTask(task);
});
expect(log).toEqual([
{microTask: false, macroTask: true, eventTask: false, change: 'macroTask', zone: 'parent'},
'macroTask', 'macroTask', {
microTask: false,
macroTask: false,
eventTask: false,
change: 'macroTask',
zone: 'parent'
}
'macroTask', 'macroTask'
]);
});

Expand Down