cleanup listener for user provided AbortSignal

[ronag]

Refs: whatwg/fetch#1287

[ronag]

The Request constructor will register an 'abort' listener we need to release at some point.

[ronag]

@benjamingr weak listeners would be nice here 😄

[flevi29]

This has been marked completed for a while now, and I assume node 18.10.0 uses the version of undici that has this issue marked as closed/completed. Running on 18.10.0 the code below gets me a MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit.

const ac = new AbortController();
for (let i = 0; i < 11; i += 1) {
  const response = await fetch("https://www.google.com", { signal: ac.signal });
  if (!response.ok) {
    throw new Error("For some reason fetch failed, make sure it doesn't");
  }
  console.log(i + 1);
}

I am writing a module for OAI-PMH lists which rely on fetching consecutively until a certain response is given. I want to be able to abort the current fetch with the same controller as I would have the previous, but currently that is not a good idea because fetch doesn't clean up the listener it registered on the signal it seems.
I guess I'll just let it leak for now, cause it does get cleaned up after fetching a whole list finishes.

[KhafraDev]

Copying from another issue: undici cleans up the listeners properly, the issue is that the signal is never being aborted and the gc won't run in the loop so the listeners aren't being removed instantly.

@ronag is there anything we can do here?

[ronag]

I don't really see a good way here... the spec doesn't really address this issue properly...

[KhafraDev]

deno has the same issue, there just isn't a warning. Should we raise/remove the limit?

[ronag]

deno has the same issue, there just isn't a warning. Should we raise/remove the limit?

I guess soo... maybe raise it to 100?

[jimmywarting]

isn't there kind of a root problem with the AbortController/AbortSignal specification in itself? not just about fetch.
i think we at node-fetch stumble upon this issue as well.

it feels to me that if the AbortController is GC then it up to the AbortController job to also remove all event listener on the AbortSignal b/c there are no longer any way for it to be aborted. so registering new listeners should also not do anything? ofc there is the method of manually dispatching own events...

feels like AbortController should create it's own internal finalizer and clean up after itself somehow.

[KhafraDev]

The abortcontroller isn't being gc'ed, each fetch adds a new abort listener to the abortsignal which is where we get the issue. Now, those listeners are eventually being gc'ed (add a globalThis.gc?.() with --expose-gc and the warnings go away), but it's not happening within the loop.

[benjamingr]

We shipped a new aborted helper in util that can solve this. It explicitly adds the abort listener weakly to enable this sort of GC - check it out.

[daveyarwood]

Could this still be an issue? See #1711.

[flevi29]

If I understand correctly this is a "won't fix", as it's more of a spec issue.