cleanup signal listener

[ronag]

When passing an signal: AbortSignal to the fetch method we register an 'abort' listener through the new Request constructor (i.e. following signal). However, the spec does not define when and where this listener should be released in order to not cause leaks in the user provided signal.

Refs: https://github.com/nodejs/undici/blob/0badd390ad5aa531a66aacee54da664468aa1577/lib/api/api-fetch/request.js#L280-L295

[annevk]

Okay, so this is specifically about https://dom.spec.whatwg.org/#abortsignal-follow which Fetch uses. That might get replaced by shaseley/abort-signal-any#1. I added a comment there so that we don't forget about this.

[benjamingr]

@ronag undici can use Node's (internal) weak event listeners which core uses for AbortSignals in some places to avoid leaks. It's not exposed to userland but like in the web the platform can use tools that end-users don't necessarily have (and hopefully eventually will)

[ronag]

We could if they were accessible in user space...

[benjamingr]

@ronag undici is shipped as part of Node isn't it? Or do you mean the userland lib itself?

Node can't expose weak event listeners outside of core (due to spec reasons, we can't extend APIs). Maybe we can ship AbortSignal.any early using that and then use that?

[ronag]

@ronag undici is shipped as part of Node isn't it? Or do you mean the userland lib itself?

It's imported into Node... so it is developed outside of node so we can't use any private node stuff. We would need to expose it somehow...

[ronag]

@KhafraDev FYI

[Jamesernator]

So AbortSignal.any has been merged but I don't believe the changes from AbortSignal.any actually do anything to affect the cleanup of the associated abort steps for fetch.

Like consider the following code:

const abortController = new AbortController();
const res = await fetch("https://example.com", { signal: abortController.signal });
const text = await res.text();
// abort algorithms should become collectable around HERE

// ...sometime later
abortController.abort();

now we would want the fetch's abort algorithm to become collectable sometime around where is marked HERE given that signal can no longer affect body. However the spec says:

A non-aborted dependent AbortSignal object must not be garbage collected while its source signals is non-empty and it has registered event listeners for its abort event or its abort algorithms is non-empty.

and these conditions are met, the internal requestObject.[[signal]] created during fetch() has a non-empty source signal list consisting of [abortController.signal] AND it has an abort algorithm which is is added by step 11 but this algorithm isn't removed.

(NOTE: requestObject is collectable as far as I can tell, however it's [[signal]] is not).

Note that this means as long as abortController.signal is retained, so will the abort algorithm. The fix here would be to remove the abort algorithm once the body is consumed (is sooner possible?).

[annevk]

cc @shaseley

[shaseley]

Yeah, I think removing the algorithm when "done" with it makes sense, and you're right it's necessary to make the signal eligible for GC. I ran into this when I was first prototyping AbortSignal.any(), and I ended up auditing all AbortSignal usage in Blink and changing things so the algorithms always get removed. Fixing this in specs is one of the follow-up issues for any() that was identified (mentioned in whatwg/dom#1194), which is on my radar to fix.