Reconnect when the receive loop died but the socket stayed open - #88
Merged
Conversation
connect() only ever checked the transport:
transport?.let { if (it.isActive && !disconnected) return it }
socketSubscription is never consulted. The receive loop runs in a scope handed
in from outside, so its lifetime is not the connection's to control: cancelling
that scope kills the loop without closing the socket. onTransportGone is not
reached (CancellationException is rethrown first), disconnected stays false and
isActive stays true, so connect() hands back a socket nobody reads. Every later
command then waits out its commandTimeout for a reply that cannot be delivered,
and failPendingRequests never fires because nothing signals the loss.
isUsable() now requires a live receive loop as well, and the rebuild branch
triggers on any unusable transport rather than on the disconnected flag alone —
a loop that died without reporting anything needs the same treatment as one that
did.
Also drops a stale sentence left in the lastMessageAt comment, which described
both 0 and null as the 'nothing arrived yet' sentinel.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
connect()only ever checked the transport:socketSubscriptionis never consulted. The receive loop runs in a scope handed in from outside, so its lifetime is not the connection's to control — cancelling that scope kills the loop without closing the socket.From there nothing notices:
onTransportGoneis not reached (theCancellationExceptionis rethrown first),disconnectedstaysfalse,isActivestaystrue.connect()hands back a socket nobody reads, so every later command waits out itscommandTimeoutfor a reply that cannot be delivered — andfailPendingRequestsnever fires, because nothing signals the loss. WithcommandTimeout <= 0the wait has no end at all.An open socket whose receive loop is dead is not a healthy connection: the loop is what completes the waiters.
Changes
isUsable()requires a live receive loop alongside an active transport, and the rebuild branch now triggers on any unusable transport rather than on thedisconnectedflag alone — a loop that died without reporting anything needs the same treatment as one that did.This closes the last gap in the set already fixed by the in-flight-command failure and
connectMutexdouble-check work.Also drops a stale sentence in the
lastMessageAtcomment, which described both0andnullas the "nothing arrived yet" sentinel.Tests
The first fails on
main. Mutation-checked: dropping the liveness check, gating the rebuild ondisconnectedalone, testing that the loop exists rather than that it is alive, and never reusing a transport each turn one red.205 JVM tests green.
kotlinStoreYarnLockand the:cdp/:core/:opentelemetrydetekt gates fail identically onmainand are untouched;:coreviolations stay at 142.