Fix connection ownership and session announcement issues - #184
Merged
Conversation
* fix(connection)!: a transition of the connection has one owner 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. * fix(connection): a retired session is announced whatever wins the transition 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. * docs(connection): the inactivity check needs UseCustomPing, and the docs 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. * fix(websocket): a failure of an established connection is reported once 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. * docs(changelog): 11.4.0.0 is dated the day it is finished, not the day it was opened
…#182) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com>
Platonenkov
enabled auto-merge
September 9, 2026 17:14
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.
No description provided.