fix(connection): a transition of the connection has one owner - #181
Merged
Conversation
Every operation that moves the connection - ChangeServer, Connect, Disconnect, DisconnectAndWaitAsync, the health check's fast reconnect, the reconnect loop and the failed-OnConnected-handler path - decided for itself what happened to the socket, and two of them running at once were reconciled by ReferenceEquals(ws, ...) checks placed after whichever await somebody had noticed. #178 added three such checks and its review found the next window each time. The connection now carries a generation. A consumer command and the fast reconnect begin one (TakeOver), taking the session, the socket, the reconnect loop, the ping timer and the message processor out of their fields in a single critical section under _transitionLock, which absorbs _disconnectLock and _reconnectStateLock. The socket callbacks, the loop and the handler-failure path continue the generation of the socket they run for. An operation that finds the generation moved on stands down after every await and every consumer callback; Disconnect() wins against anything in flight, and an attempt it overtook closes the socket it opened. The four windows of #179 are closed by that one mechanism: - ChangeServer no longer overrides a Disconnect() that landed in one of its yields; it reports NotConnectedException. A later ChangeServer or Connect() supersedes it with OperationCanceledException. - The fast reconnect captures its session and socket in the takeover, before the RestoringConnection notification, and stands down if a handler moved the client. - The reconnect loop releases its claim under the same lock OnceClose asks under, with the socket re-checked there (_reconnectLoopGeneration replaces the task reference and its IsCompleted check). - A request is written under the lock the retirement takes the socket under: SendRequestAsync pairs the socket read with the send. The two loose ends from #178: NotConnectedException carries a default message and the ImmediateFail refusal names the policy; WebSocketClient.SendMessage no longer calls Connect() on a socket that is not open, SendMessageAsync returns a Task that faults when the message could not be written, and messages are serialized whole on the socket. Found by the cold review of this change and fixed with it, because the diff rewrites the paths they live on: OnceOpen reported Connected and started a ping timer after a Disconnect() from the OnConnected handler; Connect() after a Disconnect() left _isIntentionalDisconnect set, so a failed handshake read as a user disconnect and nothing reconnected; a handshake cancelled by a takeover left its attempt timer firing forever; Connect() over a closing socket announced no session end and swept no requests. Closes #179.
…nsition Third cold-review pass, on the committed branch. Five findings, all on the paths this change rewrites, three of them regressions of the first commit: - ChangeServer and the fast reconnect, superseded by a Disconnect() before they reached NotifySessionEndedAsync, never announced the session they had retired: the retirement silences the socket's own close callback, and the disconnect does not know the session. Both announce on the way out now. - Disconnect() announces UserDisconnected itself. Left to the close callback alone, a Connect() issued right after installs a new session before the old socket's close is processed, and the callback files it as a stale session. - A takeover that finds no socket takes no session (DetachLocked): the session belongs to whoever took the socket. Connect() after Disconnect() used to retire it and announce ConnectionLost for an end that was the disconnect's. - With StopAfterMaxAttempts, the fast reconnect's catch started a second full series after the loop it had handed the sequence to ran out of attempts and reported Disconnected. A NotConnectedException from its wait means the client gave up, and the catch stands down on it. Reachable at dev. - _disconnectTcs was installed for a socket still in its handshake, which nobody completes; the next DisconnectAndWaitAsync waited out its timeout. Installed only for a socket whose close will be reported. Reachable at dev. The loop-survival test asserted the outcome before the server's close frame could arrive; it waits for the outcome now.
…ocs now say so UseCheckHealth promised a reconnect after sixty seconds without inbound data, and InactivityTimeout described silence as "the only signal available without sending traffic". The inactivity check has always run only with UseCustomPing enabled, and deliberately: an idle connection with no subscriptions receives nothing by design, so silence without keepalive pings would declare a healthy socket dead every InactivityTimeout. The behaviour stays; both docs now say what it is and why. Found by driving the Blazor test client, whose settings disable UseCustomPing, through a connection that stayed open and went silent.
Two things the Blazor test client showed, neither reachable from the .NET unit
suite, where a cancelled handshake throws OperationCanceledException and a
dropped connection arrives as a network error.
The receive loop routed a failure that was not a network error - a frame the
protocol forbids, or in the browser any failure at all, since its
ClientWebSocket says nothing recognisable - through the connection-error
callback as well as the close callback. The first is written for a handshake
that failed: it announced "Initial connection failed" for a connection that
had been up and in use, with an OnDisconnect carrying no code, and the second
reported the real close. The close callback is the only reporter now
(ReportFailureAsCloseAsync): it classifies the failure, announces the session
end once and starts the reconnect. In the browser a WebSocketException on an
open socket is a network drop - the transport going away is the one failure
it has - and an exception with no message is described by its error code.
OnConnectionFailed, should anything still reach it for an opened session,
says "Connection lost" rather than "Initial connection failed".
A handshake this side cancelled is not reported twice. The connect-attempt
timer and a takeover cancel the socket after reporting; the browser's
cancelled ConnectAsync throws WebSocketException ("ConnectFailure") rather
than OperationCanceledException, which reached the connection-error callback
as a second failure of the same attempt.
Pinned by MalformedFrameServer, which answers the first request with a frame
carrying a reserved opcode: one OnDisconnect, one OnSessionEnded, no "Initial
connection failed", and the loop brings the client back. Two on the old code.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (14)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughVersion 11.4.0.0 adds generation-based ownership for concurrent connection transitions. It updates socket send and failure handling, adds session generation metadata, documents lifecycle outcomes, and adds regression tests for races, reconnects, cancellations, and closed-socket sends. ChangesConnection ownership and contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains after the supplied checks and regression coverage. Sequence Diagram(s)sequenceDiagram
participant Client
participant TransitionLock
participant WebSocketClient
participant SessionEvents
Client->>TransitionLock: start connection transition
TransitionLock->>WebSocketClient: create or retire socket for generation
WebSocketClient-->>Client: return connection or send result
Client->>SessionEvents: report session end or connection failure
SessionEvents-->>Client: invoke consumer callback
Client->>TransitionLock: validate callback-driven transition
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 48.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 10 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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.
Closes #179 - the follow-up to #178.
What changed
Every operation that moves the connection decided for itself what happened to the socket, and two of them running at once were reconciled by
ReferenceEquals(ws, ...)checks placed after whichever await somebody had noticed. #178 added three such checks and its review found the next window each time.The connection now carries a generation (
Connection._generation). A consumer command (Connect,ChangeServer,Disconnect,DisconnectAndWaitAsync) and the health check's fast reconnect begin one throughTakeOver, which takes the session, the socket, the reconnect loop, the ping timer and the message processor out of their fields in a single critical section under_transitionLock(absorbing_disconnectLockand_reconnectStateLock). The socket callbacks, the reconnect loop and the failed-OnConnected-handler path continue the generation of the socket they run for. An operation that finds the generation moved on stands down after every await and every consumer callback;Disconnect()wins against anything in flight, and an attempt it overtook closes the socket it opened, including one created after the takeover.The four windows in the issue, each closed by that one mechanism:
OnceClose- the loop releases its claim (_reconnectLoopGeneration, replacing the task reference and itsIsCompletedcheck) under the same lockOnceClosedecides under, with the socket re-checked there.SendRequestAsyncpairs the socket read with the send under the retirement lock;WebSocketClient.SendMessageAsyncreturns aTask, and a failed write rejects the request instead of leaving it toRequestTimeout.ChangeServeroverriding a concurrentDisconnect()- it stands down and reportsNotConnectedException; superseded by anotherChangeServer/Connect()it reportsOperationCanceledException.Loose ends:
NotConnectedExceptionhas a default message and theImmediateFailrefusal names the policy;WebSocketClient.SendMessageno longer callsConnect()on a socket that is not open, and messages are serialized whole on the socket.Found by the cold review of this change, fixed with it
Two passes, five independent readers given the diff with no description of its purpose. Four older defects on the same paths, two of them reproducible at
dev:OnceOpenreportedConnectedand started a ping timer after aDisconnect()from theOnConnectedhandler.Connect()afterDisconnect()left_isIntentionalDisconnectset: a server that was down read as "closed permanently", no reconnect. The flag now follows the generation.OnConnectionFailedforever.Connect()over a closing socket announced no session end and swept no requests.Plus one regression of the first draft (awaiting the send before the promise, which would have held a caller past its timeout on a stalled write) - the send is now observed, not awaited.
Tests
New
TestUConnectionTransitionOwner(8 tests) issues the second operation from a callback the first one runs, which lands it inside the first one's yields deterministically; 6 of them fail ondev. NewTestUWebSocketClientSend(2) pins the send semantics;CloseAfterHandshakeServercloses each connection the moment its handshake completes. The reconnect-loop window is exercised in its reachable form only, as the test remarks say.Unit suite: 1278 / 1278 on net10.0 (1268 existing + 10 new); library builds on net8.0/9.0/10.0.
Version
Xrpl→ 11.4.0.0 (minor: contract changes inChangeServerandWebSocketClient.SendMessage). Design notes inspecs/2026-09-06-connection-transition-owner-design.md.Added after a third cold-review pass and a live run of the Blazor test client
ee5dff0c): aChangeServeror fast reconnect overtaken before it announced the session it retired still announces it;Disconnect()announcesUserDisconnecteditself (aConnect()right after it used to silence the close callback); a takeover without a socket takes no session; the fast reconnect stands down when the loop it handed the sequence to gave up (StopAfterMaxAttemptsmeant nothing before);_disconnectTcsis installed only for a socket whose close will be reported.Connectedand restored subscriptions. Two WASM-only defects came out of it and are fixed here (06ec9768): a failure of an established connection was reported twice (as "Initial connection failed" and as the close), and a cancelled handshake was reported a second time because the browser throwsWebSocketExceptionrather thanOperationCanceledException. Pinned byMalformedFrameServerwhere the .NET runtime can reach it.afd8db9b):UseCheckHealth/InactivityTimeoutnow say that inactivity detection needsUseCustomPing, which is what the code has always done and why.Unit suite: 1283 / 1283.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests