diff --git a/lib/fetch/request.js b/lib/fetch/request.js index d6263fd2d43..32c84bf7e6b 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -34,6 +34,7 @@ const { setMaxListeners, getEventListeners, defaultMaxListeners } = require('eve let TransformStream = globalThis.TransformStream const kInit = Symbol('init') +const kAbortController = Symbol('abortController') const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { signal.removeEventListener('abort', abort) @@ -354,12 +355,22 @@ class Request { if (signal.aborted) { ac.abort(signal.reason) } else { + // Keep a strong ref to ac while request object + // is alive. This is needed to prevent AbortController + // from being prematurely garbage collected. + // See, https://github.com/nodejs/undici/issues/1926. + this[kAbortController] = ac + + const acRef = new WeakRef(ac) const abort = function () { - ac.abort(this.reason) + const ac = acRef.deref() + if (ac !== undefined) { + ac.abort(this.reason) + } } // Third-party AbortControllers may not work with these. - // See https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619 + // See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619. try { if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) { setMaxListeners(100, signal) @@ -367,7 +378,7 @@ class Request { } catch {} signal.addEventListener('abort', abort, { once: true }) - requestFinalizer.register(this, { signal, abort }) + requestFinalizer.register(ac, { signal, abort }) } } diff --git a/package.json b/package.json index 1406e338cb5..d64ce0b9c2f 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "test": "npm run test:tap && npm run test:node-fetch && npm run test:fetch && npm run test:cookies && npm run test:wpt && npm run test:websocket && npm run test:jest && tsd", "test:cookies": "node scripts/verifyVersion 16 || tap test/cookie/*.js", "test:node-fetch": "node scripts/verifyVersion.js 16 || mocha test/node-fetch", - "test:fetch": "node scripts/verifyVersion.js 16 || (npm run build:node && tap test/fetch/*.js && tap test/webidl/*.js)", + "test:fetch": "node scripts/verifyVersion.js 16 || (npm run build:node && tap --expose-gc test/fetch/*.js && tap test/webidl/*.js)", "test:jest": "node scripts/verifyVersion.js 14 || jest", "test:tap": "tap test/*.js test/diagnostics-channel/*.js", "test:tdd": "tap test/*.js test/diagnostics-channel/*.js -w", diff --git a/test/client-keep-alive.js b/test/client-keep-alive.js index e752995f1f9..968bc50e89f 100644 --- a/test/client-keep-alive.js +++ b/test/client-keep-alive.js @@ -32,7 +32,7 @@ test('keep-alive header', (t) => { body.on('end', () => { const timeout = setTimeout(() => { t.fail() - }, 3e3) + }, 4e3) client.on('disconnect', () => { t.pass() clearTimeout(timeout) diff --git a/test/fetch/fetch-leak.js b/test/fetch/fetch-leak.js new file mode 100644 index 00000000000..e7a260208e3 --- /dev/null +++ b/test/fetch/fetch-leak.js @@ -0,0 +1,43 @@ +'use strict' + +const { test } = require('tap') +const { fetch } = require('../..') +const { createServer } = require('http') + +test('do not leak', (t) => { + t.plan(1) + + const server = createServer((req, res) => { + res.end() + }) + t.teardown(server.close.bind(server)) + + let url + let done = false + server.listen(0, function attack () { + if (done) { + return + } + url ??= new URL(`http://127.0.0.1:${server.address().port}`) + const controller = new AbortController() + fetch(url, { signal: controller.signal }) + .then(res => res.arrayBuffer()) + .then(attack) + }) + + let prev = Infinity + let count = 0 + const interval = setInterval(() => { + done = true + global.gc() + const next = process.memoryUsage().heapUsed + if (next <= prev) { + t.pass() + } else if (count++ > 10) { + t.fail() + } else { + prev = next + } + }, 1e3) + t.teardown(() => clearInterval(interval)) +})