Fix connection and subscription stability bugs - #109
Merged
Conversation
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.
Summary
This PR fixes several bugs that could cause stuck reconnect loops, missed resubscriptions, crashes, and silent event loss under realistic concurrency conditions.
Bugs fixed
1. Client stuck in
connectingstate after transport closes mid-connectWhen the WebSocket closed while a
ConnectRequestwas in flight, the transport's_onDonecallback ran synchronously (viaCompleter.sync()), calling_processDisconnect+_scheduleReconnectbefore thecatchblock in_connectInnerresumed. Thecatchthen called_processDisconnecta secondtime, which cancelled the already-scheduled reconnect timer without rescheduling
one, leaving the client permanently stuck in
connecting.Fix: Return early from
_connectInner's catch when the error isClientDisconnectedError; the reconnect timer is already running.2. Server-initiated temporary unsubscribe did not trigger resubscription
When the server sent an
Unsubscribepush with code ≥ 2500 (temporary, e.g. code2502 for state invalidation),
_handleUnsubscribecalledmoveToSubscribingtotransition the subscription but never attempted a resubscribe. Unlike a full
disconnect, the client remains connected in this case, so waiting for a reconnect
means the subscription never recovers.
Fix: Call
resubscribeOnConnect()aftermoveToSubscribing()for alltemporary server unsubscribes (code ≥ 2500 and the existing code 2502 path).
3.
_resubscribelacked a concurrency guard_resubscribe()had no mutex, so concurrent calls (e.g. from a pending retrytimer firing at the same moment as
resubscribeOnConnect()on reconnect) couldresult in two simultaneous
SubscribeRequests for the same channel.Fix: Added
_resubscribingboolean flag (set/cleared intry/finally) thatcauses any concurrent call to return immediately.
4. State not checked after
getTokenawait in_resubscribeAfter
await _config.getToken!(event)yielded, a concurrentunsubscribe()ordisconnect could change the subscription state. The code proceeded to build and
send a
SubscribeRequestregardless.Fix: Added
if (state != subscribing || client.state != connected) returnimmediately after the
getTokenawait.5.
_addUnsubscribeStateError on concurrent close + unsubscribeWhen
client.close()ran concurrently withsubscription.unsubscribe(), themoveToUnsubscribedfuture could resume aftersubscription.close()had alreadyclosed
_unsubscribedController. Calling.add()on a closed broadcastStreamControllerthrowsStateError.Fix:
_addUnsubscribenow checks!_closedbefore adding to the stream.6.
subscription.close()did not clean up state before closing streamsclose()closed the stream controllers but did not set_closed = true, cancelpending timers, error out pending
ready()futures, or set state tounsubscribedfirst. This left a window where timer callbacks and async continuations could still
try to interact with the subscription after it was destroyed.
Fix:
close()now sets_closed = true, cancels_resubscribeTimerand_refreshTimer, calls_errorReadyFutures, and setsstate = unsubscribedbeforeclosing any controllers.
7.
subscribe()did not guard against calling on a closed subscriptionAfter
client.close(), callingsubscription.subscribe()would proceed past thestate check (state was unsubscribed but
_closedwas unset), producing confusingerrors downstream.
Fix:
subscribe()now throwsClientClosedErrorwhen_closedis true.8. Missing
UnsubscribedEventwhen unsubscribe cleanup send failsIn
moveToUnsubscribed, if the cleanupUnsubscribeRequestto the server failedand
prevStatewassubscribed, the code triggered a reconnect and returnedearly — but did so without emitting the
UnsubscribedEventto the caller. Thecaller's
await unsubscribe()would complete silently with no event on the stream.Fix: Added
_addUnsubscribe(UnsubscribedEvent(code, reason))before the earlyreturn in that error path.
9.
_errorController.addafterclient.close()in_connectInner(4 sites)If
client.close()ran while_connectInnerwas awaitinggetToken,transport.open,or
getData, the continuation could fire after_errorControllerwas closed,causing a
StateError: Cannot add event after closing.Fix: Added
if (_closed) returnguards at all four affected sites:getTokenexception catchtransport.openonErrorcallbacktransport.openexception catchgetDataexception catch10. Transport decode errors crashed the stream listener
A malformed incoming frame caused
_replyDecoder.convert()to throw, whichpropagated uncaught through the WebSocket stream
onDatahandler, killing thelistener and silently stopping all further message processing.
Fix: Wrapped
_replyDecoder.convert()in a try/catch; decode errors are nowforwarded to the
onErrorcallback so the connection handles them gracefully.11.
backoffDelaypanicked withminReconnectDelay: Duration.zeroWhen
minReconnectDelaywas zero,val.toInt()evaluated to0andRandom.nextInt(0)threw aRangeError.Fix: Return
minDelayimmediately whenval <= 0.12.
_refreshTokenmissing state check aftergetTokenawait_refreshToken()on the client checked state before callinggetTokenbut notafter. A disconnect during the
getTokenawait could leave the client in anon-connected state, yet
_refreshTokenwould still proceed to send aRefreshRequestagainst the (now-null) transport.Fix: Added
if (state != State.connected) returnafter thegetTokenawait,before the
RefreshRequestis built.13.
send()used wrong protobuf message typeClient.send()was constructingprotocol.Messageinstead ofprotocol.SendRequest, causing a runtimeArgumentErrorfrom the transport'scommand encoder on every call.
Fix: Changed to
protocol.SendRequest.14.
onErrorcallback calledtransport.close()redundantlyThe
onErrorhandler insidetransport.open(...)calledtransport.close(), butby the time a stream error fires the transport is already in an error state and the
onDonepath handles cleanup. The extra close caused a double-close and surfacedas a spurious error event.
Fix: Removed the
transport.close()call from theonErrorhandler.Tests added
close()errors pendingready()futures instead of leaking themremoveSubscriptionafterclose()does not throwStateErrorunsubscribe()duringgetTokendoes not create a server-side subscription (validates_inflightcleanup path)subscribe()on a closed subscription throwsSubscriptionUnsubscribedErrorminReconnectDelaydoes not crash on retryable disconnect