fix(fetch): raise AbortSignal max event listeners

[KhafraDev]

Fixes nodejs/node#46525
Refs #939 (comment)

[codecov-commenter]

Codecov Report

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

Coverage data is based on head (7615308) compared to base (9dceb21).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1910   +/-   ##
=======================================
  Coverage   90.41%   90.42%           
=======================================
  Files          71       71           
  Lines        6096     6099    +3     
=======================================
+ Hits         5512     5515    +3     
  Misses        584      584           

Impacted Files	Coverage Δ
lib/fetch/request.js	87.98% <100.00%> (+0.15%)	⬆️

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]

Lgtm

[ricardocosta]

@KhafraDev According to remix-run/react-router#10118, this PR introduces errors when using React Router because getEventListeners doesn't support signals of type AbortSignal , whilst signal can be of that type.

Is there any issue/plan to fix this?

[KhafraDev]

getEventListeners does work on a nodejs AbortSignal, since it extends EventTarget. If you have a minimal repro that would be helpful.

[joshkel]

@KhafraDev, thank you for looking into this. However, I'm still having problems; depending on how quickly my loop runs, Node.js might add >200 event listeners, so I still get these warnings spammed to the console. I tried raising the warning limit myself (setMaxListeners(Infinity, this.controller.signal)); however, this PR overrides any warning limit that was already set.

For a repro for remix-run/react-router#10118, see https://github.com/joshkel/vitest-undici-abort-bug. The problem arises because testing frameworks such as Vitest may cause jsdom's AbortController to be used with undici's fetch.

[KhafraDev]

So it does seem like an issue in vitest - overriding some globals is bound to cause issues (see jest...). Although we should offer support for 3rd party AbortControllers, so this is a bug. We also shouldn't be changing the max listeners if the amount was changed, but there's no way of checking so there isn't much to do with that.

[camsjams]

The heavily used third-party WHATWG implementation of an AbortController interface called abort-controller, also fails the test for getEventListeners.

For those that can, just update to use native one: https://nodejs.org/dist/latest-v18.x/docs/api/globals.html#class-abortcontroller

[KhafraDev]

this was already fixed in undici v5.21.0, node v19.6.0 is using undici v5.19.1.

[camsjams]

So does mainline Node have to update?

[KhafraDev]

node v19.8+ has the fix

[camsjams]

Sorry by mainline I guess I meant LTS (Node 18)

[sangaman]

node v19.8+ has the fix

I'm still seeing this MaxListenersExceededWarning with AbortSignal on v19.8.1, it appears to be triggered by calling the native fetch a handful of times in a test suite.

[himself65]

is there any hotpatch for nodejs 18?

[ba55ie]

is there any hotpatch for nodejs 18?

You can patch globalThis.fetch with the latest version from undici:

import { fetch } from 'undici';

globalThis.fetch = fetch;

That's how I fixed our tests in node v18.15.0:

import { fetch } from 'undici';
import { beforeAll, afterAll, test } from 'your-test-lib';

let originalFetch;

beforeAll(() => {
  originalFetch = globalThis.fetch;
  globalThis.fetch = fetch;
});

afterAll(() => {
  globalThis.fetch = originalFetch;
});

// Run your tests using fetch.

[IanVS]

Looks like this is now included in node 18.16.0 as well. 👍

[thw0rted]

I'm still seeing this with Jest tests on Node 18.16.1:

(node:10904) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit

Is this PR supposed to address the warning automatically, or am I meant to be manually increasing setMaxListeners?

[Maxlitv]

Looks like this is now included in node 18.16.0 as well. 👍

I love you.