Skip to content

fix: remove per-request queue listeners in abstract session - #1102

Open
Rinse12 wants to merge 1 commit into
ipfs:mainfrom
Rinse12:fix/abstract-session-queue-listener-leak
Open

fix: remove per-request queue listeners in abstract session#1102
Rinse12 wants to merge 1 commit into
ipfs:mainfrom
Rinse12:fix/abstract-session-queue-listener-leak

Conversation

@Rinse12

@Rinse12 Rinse12 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Closes #1101.

AbstractSession.retrieve() creates one Queue per block request and registers failure / success / idle listeners on it, but the finally only calls queue.clear(), which splices the job array and touches no listeners.

Queue extends TypedEventEmitter → native EventTarget. In a browser a listener-bearing EventTarget is kept alive by its own preserved wrapper, so every queue ever created stays a GC root along with the jobs, provider records and promise chains reachable from it. Excising them from a heap graph and re-running reachability showed the retained queues rooted 53.5% of the entire live heap; the count grew at ~930/h and reached 29,273 after 31.5 h in the app where this was found.

This keeps references to the three handlers and removes them in the same finally that already removes the provider and abort listeners.

Note for testing

This change does nothing on its own — main-event@1.0.4s removeEventListener never actually detaches (achingbrain/main-event#13, fix in achingbrain/main-event#14), so these new calls are no-ops until that lands. Issue #1101 has the detail. That is also why this went unnoticed: until main-event is fixed, every removeEventListener in the libp2p/helia tree is silently doing nothing.

Notes

  • The three handlers were inline arrow functions; they are now named consts, which is the only reason the diff looks larger than three added lines.
  • The event types come from QueueJobFailure / QueueJobSuccess, already exported from @libp2p/utils, and the queues job-options type is pulled into a local alias so the listener signatures can reuse it.
  • I did not add emitEmpty.stop() / emitIdle.stop(): the debounce interval is 1 ms so a pending timer retains the queue only briefly, and it would mean reaching into @libp2p/utils internals. Happy to add it if you prefer.
  • No test added — reproducing this needs GC-root reachability in a browser rather than anything observable from JS (listenerCount() cannot see it, and under the main-event bug it reports 0 regardless). Suggestions welcome if there is a shape you would accept.
  • packages/utils does not currently typecheck from a clean npm install on main (unbuilt @helia/interface siblings); I verified this change introduces no new diagnostics in abstract-session.ts beyond the ones already present on main.

AbstractSession.retrieve() creates a Queue per block request and registers
'failure', 'success' and 'idle' listeners on it, but the finally block only calls
queue.clear() - which splices the job array and touches no listeners.

Queue extends TypedEventEmitter, which extends the native EventTarget. In a
browser a listener-bearing EventTarget is kept alive by its own preserved
wrapper, so every Queue ever created stays a GC root, along with the jobs,
provider records and promise chains reachable from it. In a long-lived tab this
grows without bound - roughly 930 retained queues per hour in the app where this
was found, 29,273 of them after 31.5 hours, carrying 2.35M Promise and 767k
AsyncGenerator objects between them.

Keep references to the three handlers and remove them in the same finally that
already removes the 'provider' and 'abort' listeners.
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.

AbstractSession never removes the listeners it adds to each per-request Queue - unbounded growth in browsers

1 participant