Warn on EventTarget maxListeners > THRESHOLD

[benjamingr]

tl;dr;

• I want to add the "max listener warning" to all event targets (and not just NodeEventTarget)
• I checked with WHATWG and we are allowed to do this.
• This is because it's very tricky to wire cancellation correctly when attaching "abort" signal handlers.

cc @jasnell and @addaleax whom are probably the most familiar with this code/question.

Also cc @mcollina since you often have smart things to say in issues related to memory leaks :]

---

Hey,

@bterlson who helps maintain the Azure SDK has been using AbortController and AbortSignal extensively and has raised a few rather severe issues with AbortControllers/Signals.

First - thanks a ton for the valuable feedback and help Brian!

The biggest one IMO pertains to a memory leak he ran into repeatedly where:

• You have one AbortSignal/AbortController bound to the process (to deal with SIGINT for example) that propagates
• Actions downstream have their own AbortController/AbortSignals and there exists code that links them up, for example: processLevelSignal.addEventListener('abort', e => childOpController.abort())
• Since the handlers are never cleaned up - memory leaks, it basically requires library authors to write tricky cancellation aware code. This is so tricky that when we implemented APIs that take signals we didn't always get it right. For example our timers/promises timeout does not clean up its signal on successful completion (added by @jasnell, whom added AbortController and is probably the most competent with this API in core and is a strong dev in general).

We already have NodeEventTarget that checks for this bug and maxListeners like EventEmitter.

I asked WHATWG if we are allowed to emit a warning based on maxListeners to the console like we do for EventEmitters.

Note - this is a problem even if the handlers are attached with { once: true } - like the above case where the issue is when the timer completes successfully.

[mcollina]

For example our timers/promises timeout does not clean up its signal on successful completion (added by @jasnell, whom added AbortController and is probably the most competent with this API in core and is a strong dev in general)

I guess this needs to be fixed.

[benjamingr]

@mcollina yeah, I will push a fix for that (it's a relatively small fix). My concern was more with the fact if someone so familiar with the API can make this error - anyone can - and userland will run into this - and I want us to help them.

[mcollina]

I want to add the "max listener warning" to all event targets (and not just NodeEventTarget)

I agree on the topic, we should match the behavior we have in EventEmitter.

[benjamingr]

One possible issue is that we can't add properties to EventTarget so it would be impossible (or at least hard, we can expose a static method I guess) to configure this threshold.

One alternative we can do is to override this on AbortSignals (and not every EventTarget) so that regular EventTargets don't get the warning but AbortSignals do.

[mcollina]

One possible issue is that we can't add properties to EventTarget so it would be impossible (or at least hard, we can expose a static method I guess) to configure this threshold.

Why?

[jasnell]

I agree that we need the warning. It was there originally before I split out NodeEventTarget but was removed from EventTarget due to the insistence that it's behavior match the browser implementation exactly or as absolutely as possible.

One possible issue is that we can't add properties to EventTarget so it would be impossible (or at least hard, we can expose a static method I guess) to configure this threshold.

Let's make it a non-enumerable, static function with a Symbol name that is not likely to collide with anything from the standard. EventTarget[events.maxListenersSymbol] = 20

[jasnell]

PR to fix the timer promises coming soon.

I'll have a PR that adds the warning to EventTarget soon after

[benjamingr]

@jasnell already opened a PR for timers/promises at #35992

Go ahead with the warning on EventTarget :] (and feel free to ping me on eventtarget stuff I am always happy to review)

[benjamingr]

@jasnell is there somewhere where it would be easier to reach you to coordinate this sort of effort? (like, do you use IRC? Facebook? something else?)

You've been doing a large chunk of the work here and I don't want to do things that would cause you more and not less work.

[jasnell]

It's usually pretty easy to reach me on Twitter DM. I also use Signal for messaging. If that works for you email me privately and I'll give you the phone number to connect there.

[benjamingr]

I'll revive my (unused) twitter account I guess 😅

[bterlson]

@benjamingr thanks much for filing this issue! I just want to clarify the scenarios under which memory leaks are likely to occur:

1. Forgetting to remove listeners

In many real world examples I've seen, signals are long-lived and reused multiple times. This is often the case even without setting up a parent/child relationship. People just find it convenient to reuse signals, such as when you have a cancel button on a loop that triggers async operations. So if you're not careful to remove handlers, a single signal can accumulate thousands and thousands of listeners.

However, there's another problem that causes handlers and signals to never be collected...

2. Linking signals

As far as I can tell, it is impossible to link signals as @benjamingr sketched in the OP without leaking memory. In order to propagate the cancellation signal downward, the parents hold a strong reference to the children in their abort handler. Children need the ability to remove the parent's abort handler, but this API offers no way to do that as far as I can tell.

This means that the common cancellation pattern of having a single top-level signal handling e.g. SIGINT with child signals passed into individual operations to handle timeouts or other more granular events will create signals that can never be collected. This is by itself a memory leak, but is also made much worse by the first issue because now you're leaking every handler as well.

Linking signals without leaking memory could be made possible by offering users a hookable way to dispose an abort handler such as a subscription. Intercepting calls to removeEventListener and using the API proposed in #35991 to check whether any listeners remain, and if not, removing the parent -> child linkage, is an approach that works in theory. However, the fetch spec requires that listeners are added by reaching directly into the signal rather than using the public API, and anyway the fetch spec never cleans up its handlers, so signals passed into fetch would live forever.

It seems to me that solving this memory leak requires some kind of change to how fetch works at least, but even more ideally, an API that makes unsubscribing more easily observable so userland implementations of linked signals can be implemented reasonably.

Hope this context is useful, and let me know if I can be of any help in these efforts!