fix(fetch): remove abort listener when request settles by ATOM00blue · Pull Request #5318 · nodejs/undici

ATOM00blue · 2026-05-21T02:35:36Z

Problem

fetch() attaches an abort listener to the passed AbortSignal on every call — in the Request constructor and in the fetch algorithm — but only removes them via a FinalizationRegistry (on GC). Reusing one signal across many requests accumulates listeners and Node emits MaxListenersExceededWarning.

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 Request constructor exposes its cleanup through an internal static accessor following the existing pattern in request.js, so no new public symbol is introduced.

Test

Added test/fetch/issue-5285.js: issues 100 fetch calls sharing one AbortController signal and asserts no abort listeners remain and no MaxListenersExceededWarning is emitted. Fails on main, passes here. Full test/fetch suite (471 tests) and node-fetch suite pass.

Closes #5285

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

Copilot

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 Request and expose it for fetch() 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 AbortSignal across many fetch() 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.

@@ -228,6 +228,15 @@ function fetch (input, init = undefined) {
    }
  )


codecov-commenter · 2026-05-21T09:17:45Z

Codecov Report

❌ Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.23%. Comparing base (74e343b) to head (7d99406).

Files with missing lines	Patch %	Lines
lib/web/fetch/index.js	94.11%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tsctx · 2026-05-23T14:31:38Z

      //    deserializedError.

      abortFetch(p, request, responseObject, controller.serializedAbortReason, controller.controller)
+      cleanupAbortListeners()


Please move this function call to abortFetch.

tsctx · 2026-05-23T14:46:37Z

+
+  // 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))


const { setImmediate } = require('node:timers/promises')

Suggested change

await new Promise((resolve) => setTimeout(resolve, 100))

await setImmediate()

tsctx · 2026-05-23T14:46:41Z

+  // 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()


Suggested change

await res.text()

await res.arrayBuffer()

This is just a personal preference. It avoids string allocation, which makes the tests faster.

mcollina

I'm surprised this does not cause regressions, but in hindsight it should be ok

lgtm but we need to wait @KhafraDev opinion too.

KhafraDev · 2026-05-28T16:44:50Z

I'm surprised this does not cause regressions, but in hindsight it should be ok

I agree. A possible issue is that abort from onRequestStart won't be called anymore if an AbortController is aborted after a request, but I'm not sure if it has any impact. We have decent test coverage for signals, GC, and cloning requests, and no WPTs have regressed either.

If this has no consequences, I think this should be upstreamed to the fetch spec. At the bare minimum, a note stating "the abort listener added can be removed here" could be added to the call sites.

tsctx · 2026-06-02T14:24:53Z

At first, I didn’t think this would work, but after thinking it through more carefully, it actually seems to work quite well.

That said, since this behavior is outside the specification, we should proceed with caution. I found one edge case that is worth considering. In this scenario, determining when it is safe to detach the signal is very similar to how garbage collection works.

Specifically, the link can only be removed when:

the fetch fails or is aborted; or
all responses, including cloned responses, have been fully consumed, or have lost all references and been garbage-collected. This should be equivalent to the fetch controller associated with the fetch being fully disposed of.

As far as I can tell, only browsers handle this edge case correctly today; it has not yet been implemented in server-side fetch. This is not an immediate problem since it is currently unimplemented, but it will likely become an issue if we decide to implement it later.

const c = new AbortController();

const r = await fetch(
  new URL("/", globalThis.location?.href ?? "https://example.com/"),
  { signal: c.signal },
);

const r2 = r.clone(); // Clone it.

await r.arrayBuffer(); // Consume the entire body first to confirm the request has ended.

c.abort(); // Abort.

console.log((await r2.text()).slice(0, 100)); // This should throw an error because they are shared.

If this is a known limitation and we are comfortable with it for now, I’m fine with moving forward. However, it is something we will need to take into account if and when we implement this in the future.

KhafraDev · 2026-06-02T15:37:49Z

If this is a known limitation and we are comfortable with it for now, I’m fine with moving forward. However, it is something we will need to take into account if and when we implement this in the future.

I can't find anything in the spec that says this case should throw. Browsers seem to be mishandling it.

outdated

Copilot AI review requested due to automatic review settings May 21, 2026 02:35

Copilot AI reviewed May 21, 2026

View reviewed changes

Comment thread test/fetch/issue-5285.js

Comment thread test/fetch/issue-5285.js

Comment thread lib/web/fetch/request.js

Comment thread lib/web/fetch/index.js

Comment on lines 211 to 229

@@ -228,6 +228,15 @@ function fetch (input, init = undefined) {

}

)

metcoder95 requested review from KhafraDev and tsctx May 21, 2026 09:03

tsctx reviewed May 23, 2026

View reviewed changes

Comment thread lib/web/fetch/index.js

mcollina approved these changes May 28, 2026

View reviewed changes

KhafraDev previously requested changes Jun 2, 2026

View reviewed changes

KhafraDev approved these changes Jun 3, 2026

View reviewed changes

		@@ -228,6 +228,15 @@ function fetch (input, init = undefined) {
		}
		)

	await new Promise((resolve) => setTimeout(resolve, 100))
	await setImmediate()

Uh oh!

Conversation

ATOM00blue commented May 21, 2026

Problem

Fix

Test

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 21, 2026

Codecov Report

Uh oh!

tsctx May 23, 2026

Choose a reason for hiding this comment

Uh oh!

tsctx May 23, 2026

Choose a reason for hiding this comment

Uh oh!

tsctx May 23, 2026

Choose a reason for hiding this comment

Uh oh!

KhafraDev Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

tsctx Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

KhafraDev commented May 28, 2026

Uh oh!

tsctx commented Jun 2, 2026

Uh oh!

KhafraDev commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

KhafraDev commented Jun 2, 2026 •

edited

Loading