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

events.once creates an error listener that is not removed on abort. #36949

Closed
draivin opened this issue Jan 15, 2021 · 3 comments
Closed

events.once creates an error listener that is not removed on abort. #36949

draivin opened this issue Jan 15, 2021 · 3 comments

Comments

@draivin
Copy link

draivin commented Jan 15, 2021

  • Version: v15.5.0
  • Platform: Microsoft Windows NT 10.0.20279.0 x64
  • Subsystem:

What steps will reproduce the bug?

let events = require('events');

let ac = new AbortController();
let e = new events.EventEmitter();

events.once(e, 'test', {signal: ac.signal});

ac.abort();

console.log(e.listeners('test'));
console.log(e.listeners('error'));

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

The error listener should be removed, so console.log should print an empty array.

What do you see instead?

console.log prints an array with one listener.

Additional information

It seems the wrong listener is being removed in the following line:

eventTargetAgnosticRemoveListener(emitter, 'error', resolver);

The error listener is called errorListener, not resolver.

@ronag
Copy link
Member

ronag commented Jan 15, 2021

@benjamingr

@benjamingr
Copy link
Member

Good catch!

@benjamingr
Copy link
Member

I'll note that it's frustrating that we support the signal option for addEventListener but not for on/once (due to performance considerations) - using that instead of removeEventListener with a reference would have made this sort of bug a lot less likely.

ruyadorno pushed a commit that referenced this issue Jan 22, 2021
Fixes: #36949

PR-URL: #36969
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit that referenced this issue Jan 25, 2021
Fixes: #36949

PR-URL: #36969
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants