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
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.
Closes #1101.
AbstractSession.retrieve()creates oneQueueper block request and registersfailure/success/idlelisteners on it, but thefinallyonly callsqueue.clear(), which splices the job array and touches no listeners.QueueextendsTypedEventEmitter→ nativeEventTarget. In a browser a listener-bearingEventTargetis 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
finallythat already removes theproviderandabortlisteners.Note for testing
This change does nothing on its own —
main-event@1.0.4sremoveEventListenernever 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: untilmain-eventis fixed, everyremoveEventListenerin the libp2p/helia tree is silently doing nothing.Notes
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.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/utilsinternals. Happy to add it if you prefer.listenerCount()cannot see it, and under themain-eventbug it reports0regardless). Suggestions welcome if there is a shape you would accept.packages/utilsdoes not currently typecheck from a cleannpm installonmain(unbuilt@helia/interfacesiblings); I verified this change introduces no new diagnostics inabstract-session.tsbeyond the ones already present onmain.