From 7312080fe20b1c83be67b59f86beea59b93e4156 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 7 Apr 2023 20:50:30 +0200 Subject: [PATCH 1/5] fetch: fix leak Fixes: https://github.com/nodejs/undici/issues/1823 Fixes: https://github.com/nodejs/node/issues/46435 --- lib/fetch/request.js | 17 ++++++++++++++--- package.json | 2 +- test/fetch-leak.js | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 test/fetch-leak.js diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 080a5d7bfa3..a734f2d42cf 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 e019ebc7d1e..fc2f24a092e 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,7 @@ "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:jest": "node scripts/verifyVersion.js 14 || jest", - "test:tap": "tap test/*.js test/diagnostics-channel/*.js", + "test:tap": "tap --expose-gc test/*.js test/diagnostics-channel/*.js", "test:tdd": "tap test/*.js test/diagnostics-channel/*.js -w", "test:typescript": "tsd && tsc test/imports/undici-import.ts", "test:websocket": "node scripts/verifyVersion.js 18 || tap test/websocket/*.js", diff --git a/test/fetch-leak.js b/test/fetch-leak.js new file mode 100644 index 00000000000..3d06a6e133e --- /dev/null +++ b/test/fetch-leak.js @@ -0,0 +1,40 @@ +'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) + }) + + const xs = [] + const interval = setInterval(() => { + global.gc() + xs.push(process.memoryUsage().heapUsed) + if (xs.length > 5) { + done = true + const final = xs.pop() // compare against final value + xs.splice(2) // skip first two values, memory can still be growing + t.ok(xs.every(x => final - x < 1e6)) + } + }, 1e3) + t.teardown(() => clearInterval(interval)) +}) From bca0eacfb589ca2d83318fc8bddb653be2cf6b94 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 8 Apr 2023 13:19:55 +0200 Subject: [PATCH 2/5] fixup --- test/fetch-leak.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/fetch-leak.js b/test/fetch-leak.js index 3d06a6e133e..a52e224b47e 100644 --- a/test/fetch-leak.js +++ b/test/fetch-leak.js @@ -25,15 +25,18 @@ test('do not leak', (t) => { .then(attack) }) - const xs = [] + let prev = Infinity + let count = 0 const interval = setInterval(() => { + done = true global.gc() - xs.push(process.memoryUsage().heapUsed) - if (xs.length > 5) { - done = true - const final = xs.pop() // compare against final value - xs.splice(2) // skip first two values, memory can still be growing - t.ok(xs.every(x => final - x < 1e6)) + const next = process.memoryUsage().heapUsed + if (next <= prev) { + t.pass() + } else if (count++ > 10) { + t.fail() + } else { + prev = next } }, 1e3) t.teardown(() => clearInterval(interval)) From 16df62ea1acd8959ee8edd6a2e069c82ddea23fd Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 8 Apr 2023 13:26:59 +0200 Subject: [PATCH 3/5] fixup --- package.json | 4 ++-- test/{ => fetch}/fetch-leak.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename test/{ => fetch}/fetch-leak.js (96%) diff --git a/package.json b/package.json index fc2f24a092e..c8003d6a114 100644 --- a/package.json +++ b/package.json @@ -49,9 +49,9 @@ "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 --expose-gc test/*.js test/diagnostics-channel/*.js", + "test:tap": "tap test/*.js test/diagnostics-channel/*.js", "test:tdd": "tap test/*.js test/diagnostics-channel/*.js -w", "test:typescript": "tsd && tsc test/imports/undici-import.ts", "test:websocket": "node scripts/verifyVersion.js 18 || tap test/websocket/*.js", diff --git a/test/fetch-leak.js b/test/fetch/fetch-leak.js similarity index 96% rename from test/fetch-leak.js rename to test/fetch/fetch-leak.js index a52e224b47e..e7a260208e3 100644 --- a/test/fetch-leak.js +++ b/test/fetch/fetch-leak.js @@ -1,7 +1,7 @@ 'use strict' const { test } = require('tap') -const { fetch } = require('..') +const { fetch } = require('../..') const { createServer } = require('http') test('do not leak', (t) => { From fd37933aa64a6e939b217ff0296314f02e6281f6 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 8 Apr 2023 13:27:52 +0200 Subject: [PATCH 4/5] fixup --- test/client-keep-alive.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From f46f5ccbc692372753885e894754b6bffbe60789 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 8 Apr 2023 13:37:57 +0200 Subject: [PATCH 5/5] fixup