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

Zone.js does not properly handle AbortSignal in addEventListener #49591

Closed
DavidANeil opened this issue Mar 25, 2023 · 4 comments
Closed

Zone.js does not properly handle AbortSignal in addEventListener #49591

DavidANeil opened this issue Mar 25, 2023 · 4 comments
Assignees
Milestone

Comments

@DavidANeil
Copy link
Contributor

Which @angular/* package(s) are the source of the bug?

zone.js

Is this a regression?

No

Description

Zone.js tries to be clever and creates a single handler for all listeners to the same event on the same target that have the same capture setting.

However, with the somewhat recent addition of the signal setting, this means that handlers can improperly registered, consider the following two tests:

const controller = new AbortController();
window.addEventListener('myEvent', () => console.log('expected'));
window.addEventListener('myEvent', () => console.log('unexpected'), {signal: controller.signal});
controller.abort();
window.dispatchEvent(new Event('myEvent'));
// LOG: 'expected'
// LOG: 'unexpected'
const controller = new AbortController();
window.addEventListener('myEvent', () => console.log('unexpected'), {signal: controller.signal});
window.addEventListener('myEvent', () => console.log('expected'));
controller.abort();
window.dispatchEvent(new Event('myEvent'));
// nothing logs

If zone.js is not installed, only 'expected' is logged in each case.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

No response

Anything else?

No response

@ngbot ngbot bot added this to the needsTriage milestone Mar 25, 2023
@DavidANeil
Copy link
Contributor Author

These are the Jasmine tests I'm not using to make sure this doesn't break for us again:

    it('should cancel secondary listeners that are aborted', () => {
        const controller = new AbortController();
        const spy1 = jasmine.createSpy('spy1');
        const spy2 = jasmine.createSpy('spy2');
        const target = new EventTarget();
        target.addEventListener('myEvent', spy1);
        target.addEventListener('myEvent', spy2, {signal: controller.signal});
        controller.abort();
        target.dispatchEvent(new Event('myEvent'));

        expect(spy1).toHaveBeenCalled();
        expect(spy2).not.toHaveBeenCalled();
    });

    it('should not cancel secondary listeners that are not aborted', () => {
        const controller = new AbortController();
        const spy1 = jasmine.createSpy('spy1');
        const spy2 = jasmine.createSpy('spy2');
        const target = new EventTarget();
        target.addEventListener('myEvent', spy1, {signal: controller.signal});
        target.addEventListener('myEvent', spy2);
        controller.abort();
        target.dispatchEvent(new Event('myEvent'));

        expect(spy1).not.toHaveBeenCalled();
        expect(spy2).toHaveBeenCalled();
    });

And I have just used this patch to workaround the issue by basically opting out of Zone entirely:

diff --git a/node_modules/zone.js/dist/zone.js b/node_modules/zone.js/dist/zone.js
index f7c28f6..87d9b43 100755
--- a/node_modules/zone.js/dist/zone.js
+++ b/node_modules/zone.js/dist/zone.js
@@ -1876,6 +1876,9 @@
                     if (!delegate) {
                         return nativeListener.apply(this, arguments);
                     }
+                    if (typeof arguments[2] === 'object' && typeof arguments[2].signal !== 'undefined') {
+                        return nativeListener.apply(this, arguments);
+                    }
                     if (isNode && eventName === 'uncaughtException') {
                         // don't patch uncaughtException of nodejs to prevent endless loop
                         return nativeListener.apply(this, arguments);

Someone who knows more about Zone.js might be able to do something better.

@JiaLiPassion JiaLiPassion self-assigned this Mar 26, 2023
@JiaLiPassion
Copy link
Contributor

@DavidANeil , thank you for posting the issue, I will check it.

JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 26, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 28, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 28, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 29, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 29, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Mar 29, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
@LinhTT46
Copy link

Thanks @JiaLiPassion for fixing this issue. However, please take a look at the commit, it seems like it is not deployed.

JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Dec 13, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Dec 13, 2023
Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.
jessicajaniuk pushed a commit that referenced this issue Dec 18, 2023
Close #49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.

PR Close #49595
@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 Jan 18, 2024
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
…9595)

Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.

PR Close angular#49595
rlmestre pushed a commit to rlmestre/angular that referenced this issue Jan 26, 2024
…9595)

Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.

PR Close angular#49595
danieljancar pushed a commit to danieljancar/angular that referenced this issue Jan 26, 2024
…9595)

Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.

PR Close angular#49595
amilamen pushed a commit to amilamen/angular that referenced this issue Jan 26, 2024
…9595)

Close angular#49591

```
const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();
```

Currently `zone.js` doesn't support the `signal` option, this PR allows
the user to use AbortContoller to remove the event listener.

PR Close angular#49595
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants