fix(zone.js): should continue to executue listeners when throw error #41562

JiaLiPassion · 2021-04-11T06:27:41Z

zone.js patches event listeners and run all event listeners together, if
one event handler throws error, the listeners afterward may not be invoked.

Reproduction:

export class AppComponent implements AfterViewInit {
  @ViewChild('btn') btn: ElementRef;
  title = 'event-error';

  constructor(private ngZone: NgZone) {}

  ngAfterViewInit() {
    this.ngZone.runOutsideAngular(() => {
      this.btn.nativeElement.addEventListener('click', () => {
        throw new Error('test1');
      });
      this.btn.nativeElement.addEventListener('click', () => {
        console.log('add eventlistener click');
      });
    });
  }
}

Until now no Angular users report this issue becuase in the ngZone, all
error will be caught and will not rethrow, so the event listeners afterward
will still continue to execute, but if the event handlers are outside of ngZone,
the error will break the execution.

This commit catch all errors, and after all event listeners finished invocation,
rethrow the errors in seperate microTasks, the reason I am using microTask here
is to handle multiple errors case.

petebacondarwin

Reviewed-for: size-tracking

packages/zone.js/lib/common/events.ts

petebacondarwin

Great - and this avoids the need for size-tracking approval!

AndrewKushnir · 2021-04-12T17:40:43Z

FYI I've started a presubmit, but it looks like we'd need to run a global one tonight as well, since this is a change in behavior as well and might potentially affect apps that rely on the current behavior. Depending on the presubmit results, we may need to consider landing this change in major/minor only (avoid patch).

Update: started global presubmit as well (link).

AndrewKushnir · 2021-04-13T04:39:25Z

@JiaLiPassion FYI presubmits (including a global one) went well.

@mhevery could you please have a look at the changes when you get a chance?

…ow error. (#41562)" (#41707) This reverts commit 6910118. Reason: this change introduces race conditions on CI. PR Close #41707

…w error (#41562)" (#41707) This reverts commit 5c48cd3. Reason: that change introduces race conditions on CI. PR Close #41707

@ViewChild

Close angular#41522 `zone.js` patches event listeners and run all event listeners together, if one event handler throws error, the listeners afterward may not be invoked. Reproduction: ``` export class AppComponent implements AfterViewInit { @ViewChild('btn') btn: ElementRef; title = 'event-error'; constructor(private ngZone: NgZone) {} ngAfterViewInit() { this.ngZone.runOutsideAngular(() => { this.btn.nativeElement.addEventListener('click', () => { throw new Error('test1'); }); this.btn.nativeElement.addEventListener('click', () => { console.log('add eventlistener click'); }); }); } } ``` Until now no Angular users report this issue becuase in the `ngZone`, all error will be caught and will not rethrow, so the event listeners afterward will still continue to execute, but if the event handlers are outside of `ngZone`, the error will break the execution. This commit catch all errors, and after all event listeners finished invocation, rethrow the errors in seperate `microTasks`, the reason I am using `microTask` here is to handle multiple errors case.

Close angular#41520. This case related to the issue angular#41522. ``` Zone.root .fork({ name: 'xhr', onHasTask(delegate, currentZone, zone, taskState) { console.log('hasMacrotask', taskState.macroTask); return delegate.hasTask(zone, taskState); }, }) .run(() => { const xhr = new XMLHttpRequest(); xhr.open('GET', 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.11.4/zone.min.js'); xhr.addEventListener('load', () => { throw new Error(); }); xhr.send(); }); ``` zone.js invoke all `onload` event handlers before change the XHR task's state from `scheduled` to `notscheduled`, so if any `onload` listener throw error, the XHR task wlll be hang to `scheduled`, and leave the macroTask status in the zone wrongly. This has been fixed in the previous commit, this commit add test to verify the case.

AndrewKushnir · 2021-04-21T02:18:45Z

Global Presubmit #4.

#41562) Close #41520. This case related to the issue #41522. ``` Zone.root .fork({ name: 'xhr', onHasTask(delegate, currentZone, zone, taskState) { console.log('hasMacrotask', taskState.macroTask); return delegate.hasTask(zone, taskState); }, }) .run(() => { const xhr = new XMLHttpRequest(); xhr.open('GET', 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.11.4/zone.min.js'); xhr.addEventListener('load', () => { throw new Error(); }); xhr.send(); }); ``` zone.js invoke all `onload` event handlers before change the XHR task's state from `scheduled` to `notscheduled`, so if any `onload` listener throw error, the XHR task wlll be hang to `scheduled`, and leave the macroTask status in the zone wrongly. This has been fixed in the previous commit, this commit add test to verify the case. PR Close #41562

@ViewChild

…41562) Close #41522 `zone.js` patches event listeners and run all event listeners together, if one event handler throws error, the listeners afterward may not be invoked. Reproduction: ``` export class AppComponent implements AfterViewInit { @ViewChild('btn') btn: ElementRef; title = 'event-error'; constructor(private ngZone: NgZone) {} ngAfterViewInit() { this.ngZone.runOutsideAngular(() => { this.btn.nativeElement.addEventListener('click', () => { throw new Error('test1'); }); this.btn.nativeElement.addEventListener('click', () => { console.log('add eventlistener click'); }); }); } } ``` Until now no Angular users report this issue becuase in the `ngZone`, all error will be caught and will not rethrow, so the event listeners afterward will still continue to execute, but if the event handlers are outside of `ngZone`, the error will break the execution. This commit catch all errors, and after all event listeners finished invocation, rethrow the errors in seperate `microTasks`, the reason I am using `microTask` here is to handle multiple errors case. PR Close #41562

#41562) Close #41520. This case related to the issue #41522. ``` Zone.root .fork({ name: 'xhr', onHasTask(delegate, currentZone, zone, taskState) { console.log('hasMacrotask', taskState.macroTask); return delegate.hasTask(zone, taskState); }, }) .run(() => { const xhr = new XMLHttpRequest(); xhr.open('GET', 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.11.4/zone.min.js'); xhr.addEventListener('load', () => { throw new Error(); }); xhr.send(); }); ``` zone.js invoke all `onload` event handlers before change the XHR task's state from `scheduled` to `notscheduled`, so if any `onload` listener throw error, the XHR task wlll be hang to `scheduled`, and leave the macroTask status in the zone wrongly. This has been fixed in the previous commit, this commit add test to verify the case. PR Close #41562

@ViewChild

…41562) Close #41522 `zone.js` patches event listeners and run all event listeners together, if one event handler throws error, the listeners afterward may not be invoked. Reproduction: ``` export class AppComponent implements AfterViewInit { @ViewChild('btn') btn: ElementRef; title = 'event-error'; constructor(private ngZone: NgZone) {} ngAfterViewInit() { this.ngZone.runOutsideAngular(() => { this.btn.nativeElement.addEventListener('click', () => { throw new Error('test1'); }); this.btn.nativeElement.addEventListener('click', () => { console.log('add eventlistener click'); }); }); } } ``` Until now no Angular users report this issue becuase in the `ngZone`, all error will be caught and will not rethrow, so the event listeners afterward will still continue to execute, but if the event handlers are outside of `ngZone`, the error will break the execution. This commit catch all errors, and after all event listeners finished invocation, rethrow the errors in seperate `microTasks`, the reason I am using `microTask` here is to handle multiple errors case. PR Close #41562

#41562) Close #41520. This case related to the issue #41522. ``` Zone.root .fork({ name: 'xhr', onHasTask(delegate, currentZone, zone, taskState) { console.log('hasMacrotask', taskState.macroTask); return delegate.hasTask(zone, taskState); }, }) .run(() => { const xhr = new XMLHttpRequest(); xhr.open('GET', 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.11.4/zone.min.js'); xhr.addEventListener('load', () => { throw new Error(); }); xhr.send(); }); ``` zone.js invoke all `onload` event handlers before change the XHR task's state from `scheduled` to `notscheduled`, so if any `onload` listener throw error, the XHR task wlll be hang to `scheduled`, and leave the macroTask status in the zone wrongly. This has been fixed in the previous commit, this commit add test to verify the case. PR Close #41562

Close angular#41867 In the previous commit angular#41562 (comment), the error thrown in the event listener will be caught and re-thrown, but there is a bug in the commit, if there is only one listener for the specified event name, the error will not be re-thrown, so this commit fixes the issue and make sure the error is re-thrown.

angular-automatic-lock-bot · 2021-05-22T16:08:24Z

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.}

JiaLiPassion requested review from mhevery and jelbourn Apr 11, 2021

google-cla bot added the cla: yes label Apr 11, 2021

JiaLiPassion force-pushed the issue-41522 branch from 4663456 to 162c9b2 Compare Apr 11, 2021

pullapprove bot requested review from atscott, IgorMinar and petebacondarwin Apr 11, 2021

JiaLiPassion force-pushed the issue-41522 branch from 162c9b2 to 0f2be09 Compare Apr 11, 2021

JiaLiPassion added comp: zones target: patch labels Apr 11, 2021

ngbot bot added this to the Backlog milestone Apr 11, 2021

petebacondarwin approved these changes Apr 11, 2021

View changes

pullapprove bot requested a review from alxhub Apr 11, 2021

petebacondarwin requested changes Apr 11, 2021

View changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

angular deleted a comment from petebacondarwin Apr 11, 2021

JiaLiPassion force-pushed the issue-41522 branch 6 times, most recently from e61d4d1 to 36d1e0d Compare Apr 12, 2021

JiaLiPassion requested a review from petebacondarwin Apr 12, 2021

petebacondarwin approved these changes Apr 12, 2021

View changes

petebacondarwin added the action: presubmit label Apr 12, 2021

AndrewKushnir added action: review and removed action: presubmit labels Apr 13, 2021

mhevery approved these changes Apr 13, 2021

View changes

AndrewKushnir removed the action: review label Apr 13, 2021

AndrewKushnir added a commit that referenced this issue Apr 19, 2021

Revert "test(zone.js): should invoke XHR task even onload handler thr…

26a0285

…ow error. (#41562)" (#41707) This reverts commit 6910118. Reason: this change introduces race conditions on CI. PR Close #41707

AndrewKushnir added a commit that referenced this issue Apr 19, 2021

Revert "fix(zone.js): should continue to executue listeners when thro…

3631934

…w error (#41562)" (#41707) This reverts commit 5c48cd3. Reason: that change introduces race conditions on CI. PR Close #41707

JiaLiPassion force-pushed the issue-41522 branch 2 times, most recently from ced63c6 to be4e7c8 Compare Apr 20, 2021

JiaLiPassion force-pushed the issue-41522 branch 2 times, most recently from dee3e2c to b5b8a06 Compare Apr 20, 2021

JiaLiPassion force-pushed the issue-41522 branch from b5b8a06 to daaef92 Compare Apr 20, 2021

JiaLiPassion removed the action: cleanup label Apr 21, 2021

AndrewKushnir added action: presubmit action: merge and removed action: presubmit labels Apr 21, 2021

jessicajaniuk closed this in angular/angular@008eaf3 Apr 21, 2021

br-star mentioned this pull request Apr 28, 2021

Thrown error no longer propagates (caught in the try-catch inside invokeTask in patchEventTarget in events.ts) #41867

Closed

JiaLiPassion mentioned this pull request Apr 29, 2021

fix(zone.js): only one listener should also throw an error #41868

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 22, 2021

Feb	MAR	Apr
	21
2021	2022	2023

angular / angular Public

fix(zone.js): should continue to executue listeners when throw error #41562

fix(zone.js): should continue to executue listeners when throw error #41562

JiaLiPassion commented Apr 11, 2021

petebacondarwin left a comment

petebacondarwin left a comment

AndrewKushnir commented Apr 12, 2021 •

edited

AndrewKushnir commented Apr 13, 2021

AndrewKushnir commented Apr 21, 2021

angular-automatic-lock-bot bot commented May 22, 2021

angular / angular Public

fix(zone.js): should continue to executue listeners when throw error #41562

fix(zone.js): should continue to executue listeners when throw error #41562

Conversation

JiaLiPassion commented Apr 11, 2021

petebacondarwin left a comment

petebacondarwin left a comment

AndrewKushnir commented Apr 12, 2021 • edited

AndrewKushnir commented Apr 13, 2021

AndrewKushnir commented Apr 21, 2021

angular-automatic-lock-bot bot commented May 22, 2021

AndrewKushnir commented Apr 12, 2021 •

edited