fix: clear _timeoutInfo in _onclose() and scope .finally() abort controller cleanup#1462
Open
fix: clear _timeoutInfo in _onclose() and scope .finally() abort controller cleanup#1462
Conversation
…roller cleanup Fixes two related cleanup gaps in Protocol._onclose(): 1. _timeoutInfo not cleared: pending setTimeout callbacks from old requests survive close() and can fire cancel() against a new transport after close()+connect(), sending spurious notifications/cancelled for request IDs the new client never sent. Fix: iterate _timeoutInfo entries calling clearTimeout(), then clear. 2. Stale .finally() can delete new connection's abort controller: the .finally() in _onrequest deletes from _requestHandlerAbortControllers by request.id without checking if the stored controller matches. After reconnect, if a new client reuses the same request ID, the old handler's .finally() deletes the new connection's abort controller. Fix: only delete if the stored value matches the captured controller. Fixes #1459
Tests that _onclose() properly clears _timeoutInfo to prevent spurious cancellation notifications after reconnect, and that the .finally() abort controller cleanup is scoped to avoid deleting a new connection's controller when request IDs overlap.
|
commit: |
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
2 participants
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.
Fixes two related cleanup gaps in
Protocol._onclose():Bug 1:
_timeoutInfonot cleared in_onclose()_onclose()methodically clears_responseHandlers,_progressHandlers,_taskProgressTokens,_pendingDebouncedNotifications, and_requestHandlerAbortControllers, but does not clear_timeoutInfo. PendingsetTimeoutcallbacks from old requests surviveclose()and can firecancel()against a new transport afterclose()+connect(), sending spuriousnotifications/cancelledfor request IDs the new client never sent.Fix: Iterate
_timeoutInfoentries callingclearTimeout()for each, then clear the map in_onclose().Bug 2: Stale
.finally()can delete new connection's abort controllerIn
_onrequest, the fire-and-forget promise chain's.finally()doesthis._requestHandlerAbortControllers.delete(request.id). Therequest.idis captured from the closure, but there is no check that the stored controller matches. After reconnect, if a new client reuses the same request ID (common since IDs start from 0), the old handler's.finally()deletes the new connection's abort controller.Fix: Capture a reference to the specific
AbortControllerin the.finally()closure and only delete from the map if the stored value matches.Impact
Both issues are pre-existing on
v1.x. Practical impact is low because (1) spurious cancellations use stale request IDs that would be ignored, (2) Protocol reuse across different clients is uncommon. However, the issues become more relevant asclose()+connect()becomes a supported reconnect pattern.Found by bughunter.
Fixes #1459