fix: remove per-request queue listeners in abstract session - #1102
Open
Rinse12 wants to merge 1 commit into
Open
fix: remove per-request queue listeners in abstract session#1102Rinse12 wants to merge 1 commit into
Rinse12 wants to merge 1 commit into
Conversation
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.
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
1 participant
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.
Important
Reframed 2026-08-12: this is hygiene, not a leak fix. The justification I originally gave
was wrong — see the retraction on #1101. A listener-bearing
EventTargetis collected inGecko; the cycle collector reclaims it. Measured with a
FinalizationRegistry, a realQueuewith all three of these listeners still attached and nothing else referencing it iscollected 100% of the time. The queues that were being retained were held by a separate
main-eventbug, and patching that releases them whether or not this PR is applied.Retracted specifically: "every queue stays a GC root", the 53.5%-of-live-heap figure, and
the ~930/h rate.
What the change is for
AbstractSession.retrieve()adds three listeners to a per-requestQueueand never removesthem, while removing the
providerandabortlisteners a few lines away in the samefinally. This makes the teardown consistent: what the method registers, the method releases.It is worth having on its own terms — cleanup should not depend on the surrounding collector
being clever enough to make the omission harmless — but I want to be straight that it does not
fix a measured leak, and you should feel free to close it if you would rather not carry the
extra bookkeeping for that.
Notes
reason the diff looks larger than three added lines.
QueueJobFailure/QueueJobSuccess, already exported from@libp2p/utils; the queue's job-options type is pulled into a local alias so the listenersignatures can reuse it.
beyond "the listener list is empty afterwards", which
listenerCount()already reports.packages/utilsdoes not typecheck from a cleannpm installonmain(unbuilt@helia/interfacesiblings); I verified this change introduces no new diagnostics inabstract-session.tsbeyond those already present.