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

Expose setMaxListeners property on AbortSignal #54758

Open
phryneas opened this issue Sep 4, 2024 · 22 comments
Open

Expose setMaxListeners property on AbortSignal #54758

phryneas opened this issue Sep 4, 2024 · 22 comments
Labels
feature request Issues that request new features to be added to Node.js. web-standards Issues and PRs related to Web APIs

Comments

@phryneas
Copy link
Contributor

phryneas commented Sep 4, 2024

What is the problem this feature will solve?

Right now, there is no stable way to change the maxEventTargetListeners of an AbortSignal without importing from node:events.

This makes it impossible to write isomorphic library code that adds more than the default 10 event listeners, without having to resort to workarounds such as

/**
 * A workaround to set the `maxListeners` property of a node EventEmitter without having to import
 * the `node:events` module, which would make the code non-portable.
 */
function setMaxListeners(maxListeners: number, emitter: any) {
  const key = Object.getOwnPropertySymbols(new AbortController().signal).find(
    (key) => key.description === "events.maxEventTargetListeners"
  );
  if (key) emitter[key] = maxListeners;
}

This relies on the events.maxEventTargetListeners symbol description, which, to my knowledge is not part of any public API and could change at any time.

What is the feature you are proposing to solve the problem?

Add a AbortSignal.setMaxListeners instance method so isomorphic code could do something like

if ('setMaxListeners' in signal) signal.setMaxListeners(20)

What alternatives have you considered?

Alternatively, if this is not an option as it could pollute a spec-defined object with additional properties, use Symbol.for instead of new Symbol for kMaxEventTargetListeners and make that a part of the official api.


I would be willing to implement this feature.

@phryneas phryneas added the feature request Issues that request new features to be added to Node.js. label Sep 4, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Sep 4, 2024
@mcollina
Copy link
Member

mcollina commented Sep 4, 2024

The right avenue to purse this change is WHATWG (the group maintaining those standards). From past experience, any time we deviate from the standard we cause more harm than good.

@phryneas
Copy link
Contributor Author

phryneas commented Sep 4, 2024

But isn't maxEventTargetListeners in itself - a limit on the number of listeners and issuing a warning - already a node-only deviation from the standard?

So this would try to explore a way to get closer to the standard, not further away.

@phryneas
Copy link
Contributor Author

phryneas commented Sep 4, 2024

By that logic, another alternative suggestion here could be:

Alternative suggestion: set no default listener count limit on AbortController

On all newly created AbortSignal instances, set the kMaxEventTargetListeners to 0 by default to align with standard behaviour - users that want a limit on event listeners can still use setMaxListeners to set a specific limit.

In the research before opening this ticket, I've seen a lot of instances where people were getting this warning with undici fetch, so this seems to be a somewhat common problem.

In my case, this is ironically caused because I want to pass an AbortSignal as the signal option to a lot of addEventListener calls so I can make sure to clean them up.

Either way, it seems that AbortSignal is just made to be reused by many fetch calls or addEventListener calls, so the current limit of 10 is easily reached by normal intended usage (and afaik not to spec).

@avivkeller avivkeller added the web-standards Issues and PRs related to Web APIs label Sep 4, 2024
@mcollina
Copy link
Member

mcollina commented Sep 4, 2024

@jasnell you added the warning in
#36001

@KhafraDev you added those getters/setters in
#47039

What do you folks think?

@KhafraDev
Copy link
Member

node shouldn't have implemented warnings or interop with EventEmitter in the first place, but we absolutely shouldn't deviate further from the spec and other implementations

@mcollina
Copy link
Member

mcollina commented Sep 4, 2024

@nodejs/web-standards what do you all think?

@mcollina
Copy link
Member

mcollina commented Sep 4, 2024

Pinging @lucacasonato too (hope you don't mind).

@lucacasonato
Copy link

Agree with @KhafraDev. If anything this should be a setter function you import from node:events that you pass the signal to as the first argument.

@jasnell
Copy link
Member

jasnell commented Sep 4, 2024

I agree. We should not extend the standard API with non-standard extensions.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

+1 (no nonstandard extensions). I do like the alternative suggestion tho (#54758 (comment))

@benjamingr
Copy link
Member

benjamingr commented Sep 4, 2024

I agree with the alternative suggestion as well

@mcollina
Copy link
Member

mcollina commented Sep 4, 2024

@jasnell are you ok for #54758 (comment)?

@benjamingr
Copy link
Member

Just so we're clear - we are within our rights from the spec's PoV to emit a warning on more than N listeners (or really anything, warnings aren't considered observable). We've clarified that several times with WHATWG explicitly to make sure.

@jasnell
Copy link
Member

jasnell commented Sep 4, 2024

I actually prefer that warning remain in place as it has actually proved useful in a couple of cases. But I won't block removing it.

@kettanaito
Copy link

You can also use setMaxListeners from node:event on any event-based API:

import { setMaxListeners } from 'node:events'

setMaxListeners(10, new EventEmitter())
setMaxListeners(10, new BroadcastChannel())
setMaxListeners(10, new AbortSignal())

@phryneas
Copy link
Contributor Author

@kettanaito assuming I'm writing a package that will never run outside of node - which was the whole trigger for this issue - I'm not, and I was looking for an isomorphic way of doing this 😅


That said, this has been sitting for a while and I'm not sure what the conclusion really was - with support for both directions, there didn't seem to be a final verdict.

Would you at this point accept a PR with my suggestion of setting kMaxEventTargetListeners by default to 0 for AbortController?

Users in search of potential performance problems or memory leaks could still enable it, but it would probably reduce friction a lot for people who want to write isomorphic code.

@KhafraDev
Copy link
Member

You can use the somewhat new --disable-warning flag.

node --disable-warning=MaxListenersExceededWarning ... will not emit the warning.

@phryneas
Copy link
Contributor Author

phryneas commented Oct 28, 2024

@KhafraDev this is something I could do as an end user, but as a library author I cannot advise all my users to turn off that warning - they might want to rely on it in different places, while not wanting to get warnings from dependency internals.

As a library author I can also not depend on the default warning number being 10 and try to split up my 15 subscribers into e.g. 7 and 8 - the user could have changed their default to 5.

The only viable options is that as a library author I

  • either don't need to worry about the warning at all (because I know it doesn't exist),
  • or that I can use a somewhat isomorphic way of disabling it (so no importing from node:*).

The ugly workaround I'm currently using is iterating getOwnPropertySymbols and looking for the symbol name, but I believe I've recently seen a PR that starts to hide symbol properties, so I'm not sure how long that will even stay a possibility.

@mcollina
Copy link
Member

@phryneas I think that library is simple to create, as I think most bundlers allow for browsers alternates, so that some code will execute in Node.js but never in the browsers, making it isomorphic.

@phryneas
Copy link
Contributor Author

phryneas commented Nov 10, 2024

So we've gone from a library with a single (and rather short) published code file to a library with 3 published code files (2 environment-specific ones, one shared), an exports field with nested conditions and a sophisticated build setup.

I might as well publish this on npm as isomorphic-abortsignal in the end, to not bloat the original library and enable others to use it without going through the same pain.

Honestly, this feels wrong - and I'm sorry that I voice my frustration here, but I feel like it needs to be said.

This is not how things should end up when node starts picking up web standards - the need for these workarounds should go down, not up :(

@mcollina
Copy link
Member

@phryneas even if we removed the warning, this will hardly trickle down to past releases. I'm not even sure we should make it semver-minor or not.

Anyhow, would you like to send a PR to remove the warning limit? I don't see much opposition in keeping it.

@phryneas
Copy link
Contributor Author

@phryneas even if we removed the warning, this will hardly trickle down to past releases. I'm not even sure we should make it semver-minor or not.

That's still a lot better than doing nothing :)

Anyhow, would you like to send a PR to remove the warning limit? I don't see much opposition in keeping it.

Will do!

phryneas added a commit to phryneas/node that referenced this issue Nov 11, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to enable
users to write isomorphic library code.
If desirable, the max event target listeners check can still be enabled
for individual `AbortSignal` instances by calling `setMaxListeners` on them.

Refs: nodejs#54758
phryneas added a commit to phryneas/node that referenced this issue Nov 29, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to enable
users to write isomorphic library code.
If desirable, the max event target listeners check can still be enabled
for individual `AbortSignal` instances by calling `setMaxListeners` on them.

Refs: nodejs#54758
phryneas added a commit to phryneas/node that referenced this issue Dec 2, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to
enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be
enabled for individual `AbortSignal` instances by calling
`setMaxListeners` on them.

Refs: nodejs#54758
nodejs-github-bot pushed a commit that referenced this issue Dec 7, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to
enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be
enabled for individual `AbortSignal` instances by calling
`setMaxListeners` on them.

Refs: #54758
PR-URL: #55816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this issue Dec 10, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to
enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be
enabled for individual `AbortSignal` instances by calling
`setMaxListeners` on them.

Refs: #54758
PR-URL: #55816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Dec 20, 2024
This change sets the default `kMaxEventTargetListeners` property for
`AbortSignal` instances to 0, disabling the check per default, to
enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be
enabled for individual `AbortSignal` instances by calling
`setMaxListeners` on them.

Refs: #54758
PR-URL: #55816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. web-standards Issues and PRs related to Web APIs
Projects
Status: Awaiting Triage
Development

No branches or pull requests

9 participants