Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't check AbortSignal maxListeners on some node versions #4045

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

don't check AbortSignal maxListeners on some node versions #4045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions lib/web/fetch/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {

const dependentControllerMap = new WeakMap()

let abortSignalHasEventHandlerLeakWarning

try {
abortSignalHasEventHandlerLeakWarning = getMaxListeners(new AbortController().signal) > 0
} catch {
abortSignalHasEventHandlerLeakWarning = false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMaxListeners throws with 0 signals (nodejs/node@752c0b8)

}

function buildAbort (acRef) {
return abort

Expand Down Expand Up @@ -424,15 +432,10 @@ class Request {
const acRef = new WeakRef(ac)
const abort = buildAbort(acRef)

// Third-party AbortControllers may not work with these.
// See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
Comment on lines -427 to -428
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't support third party AbortControllers anymore

try {
// If the max amount of listeners is equal to the default, increase it
// This is only available in node >= v19.9.0
if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) {
setMaxListeners(1500, signal)
}
} catch {}
// If the max amount of listeners is equal to the default, increase it
if (abortSignalHasEventHandlerLeakWarning && getMaxListeners(signal) === defaultMaxListeners) {
setMaxListeners(1500, signal)
}

util.addAbortListener(signal, abort)
// The third argument must be a registry key to be unregistered.
Expand Down
3 changes: 2 additions & 1 deletion test/fetch/long-lived-abort-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const http = require('node:http')
const { fetch } = require('../../')
const { once } = require('events')
const { once, setMaxListeners } = require('node:events')
const { test } = require('node:test')
const { closeServerAsPromise } = require('../utils/node-http')
const { strictEqual } = require('node:assert')
Expand Down Expand Up @@ -30,6 +30,7 @@ test('long-lived-abort-controller', { skip: true }, async (t) => {
})

const controller = new AbortController()
setMaxListeners(1500, controller.signal)

// The maxListener is set to 1500 in request.js.
// we set it to 2000 to make sure that we are not leaking event listeners.
Expand Down
Loading