fix(fetch): remove abort listener when request settles#5318
Open
ATOM00blue wants to merge 1 commit into
Open
Conversation
fetch() registers an `abort` listener on the passed AbortSignal (in both the Request constructor and the fetch algorithm) but only removed it via the FinalizationRegistry, i.e. on garbage collection. Reusing a single signal across many requests therefore accumulated listeners and Node.js emitted a MaxListenersExceededWarning. Capture the listener-removal callbacks and invoke them deterministically once the fetch settles (end-of-body, network error and abort paths) so that no listeners are leaked when a signal is reused. Closes nodejs#5285
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds deterministic cleanup for AbortSignal listeners used by fetch()/Request to prevent listener leaks and MaxListenersExceededWarning when reusing a single signal across many requests.
Changes:
- Add request-level abort-listener cleanup plumbing in
Requestand expose it forfetch()to call. - Ensure
fetch()removes abort listeners on error/abort/end-of-body settlement paths. - Add a regression test covering listener leakage when reusing one
AbortSignalacross manyfetch()calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/fetch/issue-5285.js | Adds regression test asserting no leaked abort listeners / warnings when reusing a signal. |
| lib/web/fetch/request.js | Tracks and exposes deterministic removal of the listener that ties request signal to an external signal. |
| lib/web/fetch/index.js | Calls cleanup hooks so request/fetch abort listeners are removed when the fetch settles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Allow the trailing end-of-body cleanup of the final request, which is | ||
| // scheduled in a microtask, to run before asserting. | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) |
|
|
||
| let warning = null | ||
| function onWarning (value) { | ||
| warning = value |
Comment on lines
+446
to
+462
| const removeAbortListener = util.addAbortListener(signal, abort) | ||
| // The third argument must be a registry key to be unregistered. | ||
| // Without it, you cannot unregister. | ||
| // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry | ||
| // abort is used as the unregister key. (because it is unique) | ||
| requestFinalizer.register(ac, { signal, abort }, abort) | ||
|
|
||
| // Allow the listener to be removed deterministically once the fetch | ||
| // that owns this request has settled, instead of relying solely on the | ||
| // FinalizationRegistry (i.e. garbage collection). Reusing a single | ||
| // signal across many requests would otherwise leak listeners. | ||
| // See https://github.com/nodejs/undici/issues/5285 | ||
| this.#abortCleanup = () => { | ||
| requestFinalizer.unregister(abort) | ||
| removeAbortListener() | ||
| this.#abortCleanup = null | ||
| } |
Comment on lines
211
to
229
| @@ -228,6 +228,15 @@ function fetch (input, init = undefined) { | |||
| } | |||
| ) | |||
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5318 +/- ##
=======================================
Coverage 93.22% 93.23%
=======================================
Files 110 110
Lines 36599 36642 +43
=======================================
+ Hits 34121 34162 +41
- Misses 2478 2480 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tsctx
reviewed
May 23, 2026
| // deserializedError. | ||
|
|
||
| abortFetch(p, request, responseObject, controller.serializedAbortReason, controller.controller) | ||
| cleanupAbortListeners() |
Member
There was a problem hiding this comment.
Please move this function call to abortFetch.
tsctx
reviewed
May 23, 2026
|
|
||
| // Allow the trailing end-of-body cleanup of the final request, which is | ||
| // scheduled in a microtask, to run before asserting. | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) |
Member
There was a problem hiding this comment.
const { setImmediate } = require('node:timers/promises')
Suggested change
| await new Promise((resolve) => setTimeout(resolve, 100)) | |
| await setImmediate() |
tsctx
reviewed
May 23, 2026
| // otherwise a MaxListenersExceededWarning is emitted and the listeners leak. | ||
| for (let i = 0; i < 100; i++) { | ||
| const res = await fetch(url, { signal }) | ||
| await res.text() |
Member
There was a problem hiding this comment.
Suggested change
| await res.text() | |
| await res.arrayBuffer() |
tsctx
reviewed
May 23, 2026
| processResponseEndOfBody: (response) => { | ||
| handleFetchDone(response) | ||
| cleanupAbortListeners() | ||
| }, |
Member
There was a problem hiding this comment.
There might be some unexpected behavior when cloning the response, so we need to investigate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fetch()attaches anabortlistener to the passedAbortSignalon every call — in theRequestconstructor and in the fetch algorithm — but only removes them via aFinalizationRegistry(on GC). Reusing one signal across many requests accumulates listeners and Node emitsMaxListenersExceededWarning.Fix
Capture the listener-removal callbacks and invoke them deterministically once the fetch settles, covering the end-of-body, network-error and abort paths. The
Requestconstructor exposes its cleanup through an internal static accessor following the existing pattern inrequest.js, so no new public symbol is introduced.Test
Added
test/fetch/issue-5285.js: issues 100fetchcalls sharing oneAbortControllersignal and asserts noabortlisteners remain and noMaxListenersExceededWarningis emitted. Fails onmain, passes here. Fulltest/fetchsuite (471 tests) and node-fetch suite pass.Closes #5285