Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(zone.js): cancel tasks only when they are scheduled or running #46435

Closed
wants to merge 1 commit into from

Conversation

arturovt
Copy link
Contributor

Currently, there's no check if the task (that is being canceled) has the right state.
Only scheduled and running tasks can be canceled. If the task has a non-appropriate
state, then an error will be thrown. Cancelation should not throw an error on an already
canceled task, e.g. clearTimeout does not throw errors when it's called multiple times
on the same timer.

  • Bugfix

Issue Number: #45711

Does this PR introduce a breaking change?

  • Yes
  • No

@pullapprove pullapprove bot requested a review from JiaLiPassion June 20, 2022 09:17
@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer area: zones labels Jun 20, 2022
@ngbot ngbot bot modified the milestone: Backlog Jun 20, 2022
@arturovt arturovt changed the title fix(zone.js): cancel the task when it has the right state fix(zone.js): cancel tasks only when they are scheduled or running Jun 20, 2022
@jessicajaniuk jessicajaniuk added the target: minor This PR is targeted for the next minor release label Jul 15, 2022
Copy link
Contributor

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

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

Sorry for the late response, LGTM!

@JiaLiPassion JiaLiPassion added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 10, 2022
@@ -956,6 +956,13 @@ const Zone: ZoneType = (function(global: any) {
throw new Error(
'A task can only be cancelled in the zone of creation! (Creation: ' +
(task.zone || NO_ZONE).name + '; Execution: ' + this.name + ')');

// https://github.com/angular/angular/issues/45711

Choose a reason for hiding this comment

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

nit: this comment would be better next to a test case, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Currently, there's no check if the task (that is being canceled) has the right state.
Only `scheduled` and `running` tasks can be canceled. If the task has a non-appropriate
state, then an error will be thrown. Cancelation should not throw an error on an already
canceled task, e.g. `clearTimeout` does not throw errors when it's called multiple times
on the same timer.

PR Close angular#45711
@AndrewKushnir AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 28, 2022
@jessicajaniuk jessicajaniuk removed the action: global presubmit The PR is in need of a google3 global presubmit label Oct 11, 2022
@jessicajaniuk
Copy link
Contributor

TGP is green

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit b618b5a.

@arturovt arturovt deleted the fix/45711-cancel-task branch October 11, 2022 17:37
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: zones target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants