Skip to content

fix(fetch): keep a cloned request's abort chain alive across GC - #5748

Open
Kjubikstronk wants to merge 1 commit into
nodejs:mainfrom
Kjubikstronk:clone-keeps-abort-controller
Open

fix(fetch): keep a cloned request's abort chain alive across GC#5748
Kjubikstronk wants to merge 1 commit into
nodejs:mainfrom
Kjubikstronk:clone-keeps-abort-controller

Conversation

@Kjubikstronk

Copy link
Copy Markdown

Closes #4068.

The abort travels along a chain: the source signal, then this request's controller, then the
clone's controller. Every hop is held only weakly, and clone() keeps nothing alive, so a GC
between cloning and aborting breaks the chain and the fetch hangs.

The constructor already guards against exactly this for its own controller:

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

clone() never did the same, so this applies the same reasoning there.

Two hops, not one

Worth recording, because it cost me a wrong first attempt. Retaining only the clone's own
controller is not enough and the repro still hangs: only ac.signal is handed to
fromInnerRequest, and a signal does not keep its controller alive, so that one does need
holding, but the request the clone came from is collected too and its controller is what the
source signal's listener derefs. Both have to stay reachable.

Holding the source request rather than just its controller is what makes a clone of a clone
work: each link keeps the next one alive, so intermediates can go out of scope safely.

@andreas-karlsson, this is also why holding the original request did not help you: the clone's
own controller was being collected as well, so the chain was broken at the other end too.

Tests

test/fetch/clone-abort-after-gc.js, in the style of the existing --expose-gc tests: one clone
and a chain of three with every intermediate dropped. Both hang until the test times out
without the change.

  • test/fetch/*.js with --expose-gc: 468 passed, 5 failed, and the same 5 fail on an
    unmodified tree.
  • test/fetch/request.js, test/fetch/abort.js: 35 passed.
  • test/fetch/fetch-leak.js, test/fetch/fire-and-forget.js, test/request-timeout.js: 26 passed.
    Those are the ones that would notice references being held too long.
  • eslint clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cloned requests loose abort signal on GC

1 participant