From 755952cb27810a48828b6037ee61818a193e1974 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 24 Apr 2024 11:10:03 +0200 Subject: [PATCH 1/3] Do not leak signal listeners Signed-off-by: Matteo Collina --- lib/web/fetch/request.js | 48 +++++++++++++---------- test/fetch/long-lived-abort-controller.js | 46 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 21 deletions(-) create mode 100644 test/fetch/long-lived-abort-controller.js diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 2cb8b66db90..9ebda7c1579 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(800, signal) } else if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) { - setMaxListeners(100, signal) + setMaxListeners(800, signal) } } catch {} @@ -530,7 +536,7 @@ class Request { // 40. If initBody is null and inputBody is non-null, then: if (initBody == null && inputBody != null) { - // 1. If input is unusable, then throw a TypeError. + // 1. If input is unusa0le, then throw a TypeError. if (util.isDisturbed(inputBody.stream) || inputBody.stream.locked) { throw new TypeError( 'Cannot construct a Request with a Request object that has already been used.' diff --git a/test/fetch/long-lived-abort-controller.js b/test/fetch/long-lived-abort-controller.js new file mode 100644 index 00000000000..547f7766c70 --- /dev/null +++ b/test/fetch/long-lived-abort-controller.js @@ -0,0 +1,46 @@ +'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') + +test('long-lived-abort-controller', 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 800 in request.js. + // we set it to 1000 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 < 1000; i++) { + // make request + const res = await fetch(`http://localhost:${server.address().port}`, { + signal: controller.signal + }) + + // drain body + await res.text() + } + + strictEqual(warningEmitted, false) +}) From b48ef95a156a2807f2d48ca1567118827b38f520 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 24 Apr 2024 11:33:44 +0200 Subject: [PATCH 2/3] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/dispatcher-weakref.js | 5 +++-- lib/web/fetch/request.js | 4 ++-- test/fetch/long-lived-abort-controller.js | 10 ++++++---- 3 files changed, 11 insertions(+), 8 deletions(-) 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 9ebda7c1579..6c4a75ac7bc 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -408,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(800, signal) + setMaxListeners(1500, signal) } else if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) { - setMaxListeners(800, signal) + setMaxListeners(1500, signal) } } catch {} diff --git a/test/fetch/long-lived-abort-controller.js b/test/fetch/long-lived-abort-controller.js index 547f7766c70..1518989e4fb 100644 --- a/test/fetch/long-lived-abort-controller.js +++ b/test/fetch/long-lived-abort-controller.js @@ -7,7 +7,9 @@ const { test } = require('node:test') const { closeServerAsPromise } = require('../utils/node-http') const { strictEqual } = require('node:assert') -test('long-lived-abort-controller', async (t) => { +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!') @@ -29,10 +31,10 @@ test('long-lived-abort-controller', async (t) => { const controller = new AbortController() - // The maxListener is set to 800 in request.js. - // we set it to 1000 to make sure that we are not leaking event listeners. + // 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 < 1000; i++) { + for (let i = 0; i < 2000; i++) { // make request const res = await fetch(`http://localhost:${server.address().port}`, { signal: controller.signal From 82a382ee7af17537a369ed73f37ee1bca55acd83 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 24 Apr 2024 11:41:02 +0200 Subject: [PATCH 3/3] Update lib/web/fetch/request.js Co-authored-by: Aras Abbasi --- lib/web/fetch/request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 6c4a75ac7bc..ca12576b1f0 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -536,7 +536,7 @@ class Request { // 40. If initBody is null and inputBody is non-null, then: if (initBody == null && inputBody != null) { - // 1. If input is unusa0le, then throw a TypeError. + // 1. If input is unusable, then throw a TypeError. if (util.isDisturbed(inputBody.stream) || inputBody.stream.locked) { throw new TypeError( 'Cannot construct a Request with a Request object that has already been used.'