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): should continue to executue listeners when throw error #41562

Closed
wants to merge 2 commits into from

Conversation

JiaLiPassion
Copy link
Contributor

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.

@pullapprove pullapprove bot requested a review from alxhub April 11, 2021 16:20
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved
@angular angular deleted a comment from petebacondarwin Apr 11, 2021
@JiaLiPassion JiaLiPassion force-pushed the issue-41522 branch 6 times, most recently from e61d4d1 to 36d1e0d Compare April 12, 2021 01:08
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

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

@petebacondarwin petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Apr 12, 2021
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 12, 2021

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
Copy link
Contributor

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

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

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: presubmit The PR is in need of a google3 presubmit labels Apr 13, 2021
@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 13, 2021
AndrewKushnir added a commit that referenced this pull request Apr 19, 2021
…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 pull request Apr 19, 2021
…w error (#41562)" (#41707)

This reverts commit 5c48cd3.

Reason: that change introduces race conditions on CI.

PR Close #41707
@JiaLiPassion JiaLiPassion force-pushed the issue-41522 branch 2 times, most recently from ced63c6 to be4e7c8 Compare April 20, 2021 00:37
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.
@JiaLiPassion JiaLiPassion force-pushed the issue-41522 branch 2 times, most recently from dee3e2c to b5b8a06 Compare April 20, 2021 01:22
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.
@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 21, 2021
@AndrewKushnir
Copy link
Contributor

Global Presubmit #4.

@AndrewKushnir AndrewKushnir 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: presubmit The PR is in need of a google3 presubmit labels Apr 21, 2021
jessicajaniuk pushed a commit that referenced this pull request Apr 21, 2021
#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
jessicajaniuk pushed a commit that referenced this pull request Apr 21, 2021
…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
jessicajaniuk pushed a commit that referenced this pull request Apr 21, 2021
#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
jessicajaniuk pushed a commit that referenced this pull request Apr 21, 2021
…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
jessicajaniuk pushed a commit that referenced this pull request Apr 21, 2021
#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
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 29, 2021
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.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 29, 2021
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.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 29, 2021
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.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 29, 2021
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.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 30, 2021
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.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Apr 30, 2021
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
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 May 22, 2021
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 cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[zonejs] An exception in event handler breaks execution of remaining handlers
4 participants