Skip to content

remove the abort listener from their signal when a fetch settles - #418

Open
OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-fetch-signal-listener-leak
Open

OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-fetch-signal-listener-leak

Conversation

@OsamaAnsar

Copy link
Copy Markdown

#backgroundFetch adds an abort listener to options.signal, but the only thing that removed it was the internal AbortController aborting. When a fetch finished normally the listener stayed. With a long-lived signal (an app-wide shutdown signal, say), every completed fetch left a listener behind, and through the shared closure each listener kept that fetch's value and promise alive after the entry was evicted.

Second problem, same code: a signal that was already aborted was ignored, because the listener never fires for it. fetchMethod ran with a signal that was not aborted, and the fetch() resolved normally.

Repro on Node v24.19.0 (node --expose-gc), numbers from that run:

before (7e71a1f)
abort listeners left on a shared signal after 500 completed fetches: 500
pre-aborted signal: fetch RESOLVED with "v", fetchMethod calls: 1, fetchMethod saw signal.aborted = false
200 fetches of 1MB values, max: 1, retained after gc (arrayBuffers):
  without a signal option: 1.0 MB
  with one shared signal:  199.0 MB

after
abort listeners left on a shared signal after 500 completed fetches: 0
pre-aborted signal: fetch REJECTED with signal.reason, fetchMethod calls: 1, fetchMethod saw signal.aborted = true
  without a signal option: 1.0 MB
  with one shared signal:  0.0 MB

The fix keeps the handler in a named function, adds it with a plain addEventListener, and removes it with removeEventListener in both the resolve and reject callbacks. If signal.aborted is already true, the internal controller is aborted straight away, and the abort handling in pcall also runs right after it is registered (the event would never fire for it), so the fetch rejects with the signal's reason, as it does when the signal aborts right after fetch() is called. One thing that changes: with an already-aborted signal, fetchMethod is still called, but now it gets a signal that is already aborted.

I also compared four abort scenarios before and after (abort mid-fetch, abort mid-fetch with allowStaleOnFetchAbort and a stale value, abort after the fetch completed, and the signal seen by fetchMethod after an abort) and the results are identical.

Tests are in the new test/fetch-signal-listeners.ts (new file so the tracing snapshots do not change). They check the listener count while fetches are pending and after resolve, reject, a sync fetchMethod and a throwing one; that aborting mid-fetch still aborts the fetch (also with allowStaleOnFetchAbort); the pre-aborted signal (with status, and with ignoreFetchAbort); and a WeakRef + gc() check that evicted values are not kept alive by a shared signal (skipped without --expose-gc, which .taprc already sets). With only the source change reverted, 12 assertions in those tests fail (the listener count, pre-aborted and WeakRef ones); the two abort-mid-fetch tests pass both ways, as they are there to guard the existing behavior. With the fix, all 34 pass.

This touches the same area as #415 (staleFetchTimeout). The two do not conflict in intent, but this patch does not apply as-is on top of #415's branch because of the context lines at the top of the fetch callback, so whichever lands second needs a small rebase.

Checked on Node v24.19.0 on Windows: full tap -t0 run (coverage enforcement left on) passes with 0 failures, oxlint src test reports 0 warnings and 0 errors, and prettier --check is clean. I did not run Linux or macOS, other Node versions, or a browser. (On this machine I ran tap with a small preload that falls back to junctions for symlink, and installed @tapjs/clock with --no-save, see #413. Neither is part of this PR.)

fetch() added an abort listener to options.signal that was only removed
if the internal AbortController aborted, never when the fetch finished.
With a long-lived signal (an app-wide shutdown signal, say), every
completed fetch left a listener behind, and through the shared closure
those listeners kept the fetched values and promises alive after they
were evicted from the cache.

The listener is now removed when the fetch resolves or rejects.

Also, a signal that was already aborted was ignored, because the listener
never fired: fetchMethod ran with a signal that was not aborted, and the
fetch resolved normally. Now the fetch rejects with the signal's reason,
and fetchMethod is passed a signal that is already aborted.

This branch has not been deployed

No deployments
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.

1 participant