Skip to content

Commit

Permalink
fetch: fix leak
Browse files Browse the repository at this point in the history
Fixes: #1823
Fixes: nodejs/node#46435
  • Loading branch information
ronag committed Apr 8, 2023
1 parent f0271d4 commit 7312080
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 4 deletions.
17 changes: 14 additions & 3 deletions lib/fetch/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -354,20 +355,30 @@ 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)
}
} catch {}

signal.addEventListener('abort', abort, { once: true })
requestFinalizer.register(this, { signal, abort })
requestFinalizer.register(ac, { signal, abort })
}
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
40 changes: 40 additions & 0 deletions test/fetch-leak.js
Original file line number Diff line number Diff line change
@@ -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))
})

0 comments on commit 7312080

Please sign in to comment.