fix(fetch): remove terminated event listener on redirect

[KhafraDev]

Disclaimer: this is a pretty terrible solution, but I couldn't find another way of fixing this.

Fixes #1711

• The listener gets added here
  

  undici/lib/fetch/index.js

  Line 1813 in 7102eb0

  fetchParams.controller.on('terminated', onAborted)

• This is where it should be removed (& onError). If we remove it here it doesn't work, my best guess is that it gets gc'ed? I can't move onAborted outside of this scope as it relies on response, and if we use a closure (etc.) the same problem would happen.
  

  undici/lib/fetch/index.js

  Line 2028 in 7102eb0

  onComplete () {

If you have any idea of how to fix this, please let me know. I spent maybe 45 minutes trying anything I could think of.

Also not sure how I would add tests for this.

[codecov-commenter]

Codecov Report

Base: 89.55% // Head: 89.55% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (394b2ad) compared to base (145358d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1715   +/-   ##
=======================================
  Coverage   89.55%   89.55%           
=======================================
  Files          58       58           
  Lines        5257     5258    +1     
=======================================
+ Hits         4708     4709    +1     
  Misses        549      549           

Impacted Files	Coverage Δ
lib/fetch/index.js	83.39% <100.00%> (+0.03%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mcollina]

Could you add a unit test?

[mcollina]

The code looks good. I would remove the individual listener and not all.

[ronag]

For now... I think it's easier just to increase the listener limit, i.e. setMaxListeners.

[KhafraDev]

I would remove the individual listener and not all.

I tried doing this in the dispatch function and it wouldn't remove the listener. Moving onAborted outside of that block allowed it to be removed, but we need access to response to set the aborted flag.

Could you add a unit test?

Is there an easy way of checking if a warning was emitted?

[jonseymour]

Did this PR actually fix anything or did it just mask the problem so that an annoying message got printed less frequently?

[KhafraDev]

Masked the issue.

[KhafraDev]

I'll have a PR that actually fixes it, wasn't that complicated.

[jonseymour]

Could I clarify, Is there is an actual issue with event listeners being added but not removed in the presence of redirect responses?

[Uzlopak]

its not like you have a memory leak. When fetch is ending, it garbage collects the instance and all the open listeners are garbage collected too.

[jonseymour]

ok, so the emission of the messages was simply caused by bursty acquisition of listeners prior to GC occurring?

[Uzlopak]

Basically. An EventEmitter could have hundreds of listeners for a single event without being an issue. But the case is very rare, so the node core devs decided to warn the devs if they add more than 10 listeners to an event without maybe actively noticing it, as it indicates a bug, which could slow down the performance, without being that obvious while you are developing.