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

doc/test(task): add task lifecycle doc and testcases to explain task state transition. #651

Merged
merged 2 commits into from
Mar 13, 2017

Conversation

JiaLiPassion
Copy link
Collaborator

@JiaLiPassion JiaLiPassion commented Feb 25, 2017

  • add doc and state transition graph to explain the ZoneTask's lifecycle.
  • add test cases to test the possible transitions described in the doc.

@mhevery, could you help me to review the document and test cases, and I have several questions here.

  1. should the ZoneTask can be only cancelled in the zone which created the task?
const task = Zone.current.scheduleMacoTask();
// currently it is ok that task being canceled by other zone.
Zone.current.fork({name:'anotherZone'}).cancelTask(task);
  1. when overriding task.zone in onScheduleTask callback of ZoneSpec, the hasTask callback will still be called in original zoneSpec, is that the correct behavior?

https://github.com/JiaLiPassion/zone.js/blob/5fe8a83df6e5197a5a1fe98f59481d83909168d3/test/common/task.spec.ts#L921

  1. in the test case you write here, https://github.com/angular/zone.js/blob/master/test/common/zone.spec.ts#L226, the zone of task is override in onScheduleTask callback, and in current implementation, I can also override it in onInvokeTask callback or onCancelTask callback, if I do so, is that the correct behavior?

  2. when error occurs in onHasTask, should we just try/catch it here https://github.com/angular/zone.js/blob/master/lib/zone.ts#L1046, because I think it should not affect the state transition of task.

  3. if zoneSpec.onCancelTask throw error, currently the task's state will be 'canceling', should it be reset to 'scheduled' or 'running'?
    https://github.com/JiaLiPassion/zone.js/blob/5fe8a83df6e5197a5a1fe98f59481d83909168d3/test/common/task.spec.ts#L148

  4. for non periodical macroTask(such as setTimeout), should we allow the task be canceled after running? now the logic in common/timer.ts add the check logic to make sure the non periodical macro task can be only canceled before running. Should we move those logic into zone.ts?
    https://github.com/angular/zone.js/blob/master/lib/common/timers.ts#L74

  5. Should we check when application want to cancel a microTask?

  6. should we do some parent-child relation check when reschedule task, for example we have zoneParent, zoneChild, zoneGrandChild, and in zoneChild, application want to reschedule task to zoneGrandChild, it will cause infinite loop. should we check it in zone.ts?

Thank you.

@JiaLiPassion JiaLiPassion changed the title doc(task): add task lifecycle documentation to explain state transition of Zone Task. doc/test(task): add task lifecycle doc and testcases to explain task state transition. Feb 26, 2017
@mhevery mhevery force-pushed the master branch 12 times, most recently from b55b4ad to f3b8885 Compare March 7, 2017 18:05
@mhevery
Copy link
Contributor

mhevery commented Mar 8, 2017

  1. should the ZoneTask can be only cancelled in the zone which created the task?

Yes, we should add code which enforces that is the case.

  1. when overriding task.zone in onScheduleTask callback of ZoneSpec, the hasTask callback will still be called in original zoneSpec, is that the correct behavior?

No, the hasTask should only be run on the zone which the task was scheduled. If the scheduling process was canceled than it should not run hasTask.

Your test is is assigning task.zone = ... which is not allowed. We should make that read only.

  1. in the test case you write here, https://github.com/angular/zone.js/blob/master/test/common/zone.spec.ts#L226, the zone of task is override in onScheduleTask callback, and in current implementation, I can also override it in onInvokeTask callback or onCancelTask callback, if I do so, is that the correct behavior?

That test is wrong and should be fixed and we should make task.zone readonly. See above

  1. when error occurs in onHasTask, should we just try/catch it here https://github.com/angular/zone.js/blob/master/lib/zone.ts#L1046, because I think it should not affect the state transition of task.

Good question. I am not clear on that. An error here should not prevent other hasTasks from running, so I think we should try catch and send the error to the current zone onError handler.

  1. if zoneSpec.onCancelTask throw error, currently the task's state will be 'canceling', should it be reset to 'scheduled' or 'running'?
    https://github.com/JiaLiPassion/zone.js/blob/5fe8a83df6e5197a5a1fe98f59481d83909168d3/test/common/task.spec.ts#L148

Good catch. Not sure what the correct behavior is. Since we don't know if we succeeded in canceling it. So the task is in unknown state. I think we should add a new state unknown???.

  1. for non periodical macroTask(such as setTimeout), should we allow the task be canceled after running? now the logic in common/timer.ts add the check logic to make sure the non periodical macro task can be only canceled before running. Should we move those logic into zone.ts?
    https://github.com/angular/zone.js/blob/master/lib/common/timers.ts#L74

So you are suggesting that canceling a canceled task should be a noop. In general I don't like silent ignores. I think it is an error to cancel a canceled task. So I would keep it the way it is even though the place where it matters setTimeout works the other way.

  1. Should we check when application want to cancel a microTask?

Tell me more, I am not sure what you are getting to.

  1. should we do some parent-child relation check when reschedule task, for example we have zoneParent, zoneChild, zoneGrandChild, and in zoneChild, application want to reschedule task to zoneGrandChild, it will cause infinite loop. should we check it in zone.ts?

I ran into it. Yes, that would be a useful check.

@JiaLiPassion
Copy link
Collaborator Author

JiaLiPassion commented Mar 8, 2017

should the ZoneTask can be only cancelled in the zone which created the task?
Yes, we should add code which enforces that is the case.
Ok, I will add check of it.

when overriding task.zone in onScheduleTask callback of ZoneSpec, the hasTask callback will still be called in original zoneSpec, is that the correct behavior?
No, the hasTask should only be run on the zone which the task was scheduled. If the scheduling process was canceled than it should not run hasTask.

Your test is is assigning task.zone = ... which is not allowed. We should make that read only.

in the test case you write here, https://github.com/angular/zone.js/blob/master/test/common/zone.spec.ts#L226, the zone of task is override in onScheduleTask callback, and in current implementation, I can also override it in onInvokeTask callback or onCancelTask callback, if I do so, is that the correct behavior?
That test is wrong and should be fixed and we should make task.zone readonly. See above

so zone.js don't support override zone, a task can only change zone by reschedule when task in scheduling state, is that right?

when error occurs in onHasTask, should we just try/catch it here https://github.com/angular/zone.js/blob/master/lib/zone.ts#L1046, because I think it should not affect the state transition of task.
Good question. I am not clear on that. An error here should not prevent other hasTasks from running, so I think we should try catch and send the error to the current zone onError handler.

Ok, got it ,I will implement it.

if zoneSpec.onCancelTask throw error, currently the task's state will be 'canceling', should it be reset to 'scheduled' or 'running'?
https://github.com/JiaLiPassion/zone.js/blob/5fe8a83df6e5197a5a1fe98f59481d83909168d3/test/common/task.spec.ts#L148
Good catch. Not sure what the correct behavior is. Since we don't know if we succeeded in canceling it. So the task is in unknown state. I think we should add a new state unknown???.

In fact, I also want to confirm if zoneSpec.onCancelTask throw error, can the task be cancel again?
if the task is in unknown state, it should not be able to be cancelled again.

for non periodical macroTask(such as setTimeout), should we allow the task be canceled after running? now the logic in common/timer.ts add the check logic to make sure the non periodical macro task can be only canceled before running. Should we move those logic into zone.ts?
https://github.com/angular/zone.js/blob/master/lib/common/timers.ts#L74
So you are suggesting that canceling a canceled task should be a noop. In general I don't like silent ignores. I think it is an error to cancel a canceled task. So I would keep it the way it is even though the place where it matters setTimeout works the other way.

Ok, got it.

Should we check when application want to cancel a microTask?
Tell me more, I am not sure what you are getting to.

Now microTask doesn't have customCancel function, and for the user scenario, microTask like promise, process.nextTick, should not be able to be cancelled, so I want to confirm should we add check that if user want to zone.cancelTask(microTask).

should we do some parent-child relation check when reschedule task, for example we have zoneParent, zoneChild, zoneGrandChild, and in zoneChild, application want to reschedule task to zoneGrandChild, it will cause infinite loop. should we check it in zone.ts?
I ran into it. Yes, that would be a useful check.

Ok, got it , I will add the check

@JiaLiPassion JiaLiPassion force-pushed the taskdoc branch 3 times, most recently from 585bf66 to 2045a16 Compare March 12, 2017 17:38
@mhevery
Copy link
Contributor

mhevery commented Mar 13, 2017

so zone.js don't support override zone, a task can only change zone by reschedule when task in scheduling state, is that right?

correct

In fact, I also want to confirm if zoneSpec.onCancelTask throw error, can the task be cancel again?
if the task is in unknown state, it should not be able to be cancelled again.

correct

Now microTask doesn't have customCancel function, and for the user scenario, microTask like promise, process.nextTick, should not be able to be cancelled, so I want to confirm should we add check that if user want to zone.cancelTask(microTask).

I think we should throw if you try to cancel a microtask, since microtask has no cancel function.

@mhevery
Copy link
Contributor

mhevery commented Mar 13, 2017

Let me know when you think this PR is ready to be merged in, and I will merge it.

@JiaLiPassion
Copy link
Collaborator Author

@mhevery , I have updated the code, don't updateTaskCount when onCancelTask throw error, please review. the other comments are all done!

@mhevery mhevery merged commit ef39a44 into angular:master Mar 13, 2017
@JiaLiPassion JiaLiPassion deleted the taskdoc branch March 17, 2017 03:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants