Skip to content

Expose setMaxListeners property on AbortSignal #54758

Description

@phryneas

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.

Activity

  1. mcollina commented on Sep 4, 2024

    @mcollina
    SponsorMember

    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.

  2. phryneas commented on Sep 4, 2024

    @phryneas
    ContributorAuthor

    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.

  3. phryneas commented on Sep 4, 2024

    @phryneas
    ContributorAuthor

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

  4. added
    web-standardsIssues and PRs related to web-platform APIs and standards compliance.
    on Sep 4, 2024
  5. mcollina commented on Sep 4, 2024

    @mcollina
    SponsorMember

    @jasnell you added the warning in
    #36001

    @KhafraDev you added those getters/setters in
    #47039

    What do you folks think?

  6. KhafraDev commented on Sep 4, 2024

    @KhafraDev
    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

  7. mcollina commented on Sep 4, 2024

    @mcollina
    SponsorMember

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

  8. mcollina commented on Sep 4, 2024

    @mcollina
    SponsorMember

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

  9. lucacasonato commented on Sep 4, 2024

    @lucacasonato

    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.

  10. jasnell commented on Sep 4, 2024

    @jasnell
    Member

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

  11. ljharb commented on Sep 4, 2024

    @ljharb
    SponsorMember

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

  12. benjamingr commented on Sep 4, 2024

    @benjamingr
    Member

    I agree with the alternative suggestion as well

  13. 7 remaining items

  14. mcollina commented on Nov 10, 2024

    @mcollina
    SponsorMember

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

  15. phryneas commented on Nov 10, 2024

    @phryneas
    ContributorAuthor

    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 :(

  16. mcollina commented on Nov 11, 2024

    @mcollina
    SponsorMember

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

  17. phryneas commented on Nov 11, 2024

    @phryneas
    ContributorAuthor

    @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!

  18. github-actions commented on May 11, 2025

    @github-actions
    Contributor

    There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Issues and PRs exempt from automated stale handling. label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
    For more information on how the project manages feature requests, please consult the feature request management document.

  19. added
    staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.
    on May 11, 2025
  20. phryneas commented on May 11, 2025

    @phryneas
    ContributorAuthor

    Solved for my use case via #55816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    feature requestIssues requesting new Node.js features.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.web-standardsIssues and PRs related to web-platform APIs and standards compliance.

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions