diff --git a/lib/web/fetch/dispatcher-weakref.js b/lib/web/fetch/dispatcher-weakref.js index 05fde6f09f4..6ac5f374992 100644 --- a/lib/web/fetch/dispatcher-weakref.js +++ b/lib/web/fetch/dispatcher-weakref.js @@ -33,9 +33,10 @@ class CompatFinalizer { } module.exports = function () { - // FIXME: remove workaround when the Node bug is fixed + // FIXME: remove workaround when the Node bug is backported to v18 // https://github.com/nodejs/node/issues/49344#issuecomment-1741776308 - if (process.env.NODE_V8_COVERAGE) { + if (process.env.NODE_V8_COVERAGE && process.version.startsWith('v18')) { + process._rawDebug('Using compatibility WeakRef and FinalizationRegistry') return { WeakRef: CompatWeakRef, FinalizationRegistry: CompatFinalizer diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 2cb8b66db90..ca12576b1f0 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -38,6 +38,29 @@ const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { signal.removeEventListener('abort', abort) }) +function buildAbort (acRef) { + return abort + + function abort () { + const ac = acRef.deref() + if (ac !== undefined) { + // Currently, there is a problem with FinalizationRegistry. + // https://github.com/nodejs/node/issues/49344 + // https://github.com/nodejs/node/issues/47748 + // In the case of abort, the first step is to unregister from it. + // If the controller can refer to it, it is still registered. + // It will be removed in the future. + requestFinalizer.unregister(abort) + + // Unsubscribe a listener. + // FinalizationRegistry will no longer be called, so this must be done. + this.removeEventListener('abort', abort) + + ac.abort(this.reason) + } + } +} + let patchMethodWarning = false // https://fetch.spec.whatwg.org/#request-class @@ -377,24 +400,7 @@ class Request { this[kAbortController] = ac const acRef = new WeakRef(ac) - const abort = function () { - const ac = acRef.deref() - if (ac !== undefined) { - // Currently, there is a problem with FinalizationRegistry. - // https://github.com/nodejs/node/issues/49344 - // https://github.com/nodejs/node/issues/47748 - // In the case of abort, the first step is to unregister from it. - // If the controller can refer to it, it is still registered. - // It will be removed in the future. - requestFinalizer.unregister(abort) - - // Unsubscribe a listener. - // FinalizationRegistry will no longer be called, so this must be done. - this.removeEventListener('abort', abort) - - ac.abort(this.reason) - } - } + const abort = buildAbort(acRef) // Third-party AbortControllers may not work with these. // See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619. @@ -402,9 +408,9 @@ class Request { // 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(100, signal) + setMaxListeners(1500, signal) } else if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) { - setMaxListeners(100, signal) + setMaxListeners(1500, signal) } } catch {} diff --git a/test/fetch/long-lived-abort-controller.js b/test/fetch/long-lived-abort-controller.js new file mode 100644 index 00000000000..1518989e4fb --- /dev/null +++ b/test/fetch/long-lived-abort-controller.js @@ -0,0 +1,48 @@ +'use strict' + +const http = require('node:http') +const { fetch } = require('../../') +const { once } = require('events') +const { test } = require('node:test') +const { closeServerAsPromise } = require('../utils/node-http') +const { strictEqual } = require('node:assert') + +const isNode18 = process.version.startsWith('v18') + +test('long-lived-abort-controller', { skip: isNode18 }, async (t) => { + const server = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }) + res.write('Hello World!') + res.end() + }).listen(0) + + await once(server, 'listening') + + t.after(closeServerAsPromise(server)) + + let warningEmitted = false + function onWarning () { + warningEmitted = true + } + process.on('warning', onWarning) + t.after(() => { + process.off('warning', onWarning) + }) + + const controller = new AbortController() + + // The maxListener is set to 1500 in request.js. + // we set it to 2000 to make sure that we are not leaking event listeners. + // Unfortunately we are relying on GC and implementation details here. + for (let i = 0; i < 2000; i++) { + // make request + const res = await fetch(`http://localhost:${server.address().port}`, { + signal: controller.signal + }) + + // drain body + await res.text() + } + + strictEqual(warningEmitted, false) +})