diff --git a/.ci-config/Dockerfile.nightly b/.ci-config/Dockerfile.nightly index 4a0a0773..5db1ae85 100644 --- a/.ci-config/Dockerfile.nightly +++ b/.ci-config/Dockerfile.nightly @@ -10,7 +10,7 @@ RUN install -m 0755 -d /usr/share/keyrings \ # rippled was renamed to xrpld on the develop branch; the nightly channel publishes it as the "xrpld" package. # The version must be pinned: the timestamp format changed from 14 to 12 digits mid-2026, # so Debian version ordering ranks old 14-digit builds above the newer 12-digit ones. -ARG XRPLD_VERSION=3.4.0~b0+202608111815.26cc683e-1 +ARG XRPLD_VERSION=3.4.0~rc1+202609050006.e3c8996e-1 RUN echo "deb [signed-by=/usr/share/keyrings/ripple-key.gpg] https://repos.ripple.com/repos/rippled-deb jammy nightly" > /etc/apt/sources.list.d/ripple.list \ && apt-get update \ && apt-get install -y --no-install-recommends --allow-downgrades "xrpld=${XRPLD_VERSION}" \ diff --git a/.ci-config/rippled.batchv11.cfg b/.ci-config/rippled.batchv11.cfg index f2999f3c..a409f107 100644 --- a/.ci-config/rippled.batchv11.cfg +++ b/.ci-config/rippled.batchv11.cfg @@ -102,6 +102,7 @@ DeepFreeze DynamicMPT DynamicNFT LendingProtocol +LendingProtocolV1_1 MPTokensV1 MPTokensV2 NFTokenMintOffer @@ -122,6 +123,7 @@ fixCleanup3_1_3 fixCleanup3_2_0 fixCleanup3_3_0 fixCleanup3_4_0 +fixCleanup3_5_0 fixDirectoryLimit fixEmptyDID fixEnforceNFTokenTrustline @@ -163,6 +165,7 @@ DAF3A6EB04FA5DC51E8E4F23E9B7022B693EFA636F23F22664746C77B5786B23 DeepFreeze 58E92F338758479C06084E1B6BA366BAD8F75E5329A7F0EEAFFFDA51E5106B7F DynamicMPT C1CE18F2A268E6A849C27B3DE485006771B4C01B2FCEC4F18356FE92ECD6BB74 DynamicNFT 565B90CA1AB2B9D42208ED10884188C64F9E19083DECB9634AAF06EB03299509 LendingProtocol +A360E2BFD775A5B0DCE1C36C16DF31B72735A57584FD163655D2F9564F8E7AC8 LendingProtocolV1_1 950AE2EA4654E47F04AA8739C0B214E242097E802FD372D24047A89AB1F5EC38 MPTokensV1 EE3CF852F0506782D05E65D49E5DCC3D16D50898CD1B646BAE274863401CC3CE NFTokenMintOffer 0F48FF561C709540328F31F1C97FD512ACC8B4E42138A161CB0E21ECA292540B PermissionDelegationV1_1 @@ -182,6 +185,7 @@ C98D98EE9616ACD36E81FDEB8D41D349BF5F1B41DD64A0ABC1FE9AA5EA267E9C XChainBridge 21B8D2F76F68E11E9C077A43BBBC394136E9987E99DDB73966DD68419467E431 fixCleanup3_2_0 3298D47E1F3A8A24FECAA30F699B8FE1DD234E072834BA099AD8180FFCE0FEC4 fixCleanup3_3_0 98433DD001A5737F773D74F8CA2A25A065089C73B2E611C760BAF369E4FECA76 fixCleanup3_4_0 +7300E10109D19BF1E87ACE63D3A79CD4ED6B9851C37D93ED94DE9BD41CF56835 fixCleanup3_5_0 41765F664A8D67FF03DDB1C1A893DE6273690BA340A6C2B07C8D29D0DD013D3A fixDirectoryLimit 755C971C29971C9F20C6F080F2ED96F87884E40AD19554A5EBECDCEC8A1F77FE fixEmptyDID 763C37B352BE8C7A04E810F8E462644C45AFEAD624BF3894A08E5C917CF9FF39 fixEnforceNFTokenTrustline diff --git a/CHANGES.md b/CHANGES.md index e9009d48..6153cefc 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,20 @@ # Changes +## 11.4.0.0 07/09/2026 + +* **A transition of the connection has one owner** (#179, the follow-up to #178). Every operation that moves the connection - `ChangeServer`, `Connect`, `Disconnect`, `DisconnectAndWaitAsync`, the health check's fast reconnect, the reconnect loop and the path taken when an `OnConnected` handler fails - used to decide 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 checks were right where they were; the pattern was what did not scale. + * the connection now carries a generation. A consumer command and the fast reconnect begin one, taking the session, the socket, the reconnect loop, the ping timer and the message processor out of their fields in a single critical section; 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 after every consumer callback - and the one that moved it owns the rest. `Disconnect()` wins against anything in flight, and an attempt it overtook closes the socket it opened, whether the takeover found that socket installed or the socket came into being afterwards + * the four windows the issue lists are closed by that one mechanism. A `Disconnect()` landing in one of `ChangeServer`'s yields no longer gets overridden by the switch resetting it and connecting - the client was online after the consumer took it down. A status handler that answers `RestoringConnection` with a `ChangeServer` no longer has its replacement session marked retiring by the fast reconnect that ran the handler. The reconnect loop releases its claim under the same lock the close callback asks under, with the socket re-checked there, so a close processed as the loop exits either sees the loop released or is handled by it - nobody reconnecting is no longer an outcome. And a request is written under the lock the retirement takes the socket under, so a retirement finds it either not yet sent, and refused, or already handed to the socket - it no longer reaches a server the client has left + * for consumers: a `ChangeServer` that a later operation overtook reports it instead of returning success from a server the client is not on - `NotConnectedException` when a `Disconnect()` won, `OperationCanceledException` when another `ChangeServer` or a `Connect()` did. `Connect()` keeps its contract: it returns when the client is connected, wherever a concurrent switch took it, and `OperationCanceledException` still means the caller's own token. Options handed to `ChangeServer` are validated before the old connection is torn down rather than after + * the two loose ends from #178 are tied. `NotConnectedException` thrown bare carries a message that says what it is, and the immediate refusal under `RequestFailurePolicy.ImmediateFail` names the policy - since #178 that is the exception a request issued during a switch gets, where it used to get a `TimeoutException` with "Timeout" in it, and a consumer classifying by text had nothing to recognise. `WebSocketClient.SendMessage` no longer answers a socket that is not open with a `Connect()` - `ConnectAsync` on an already used `ClientWebSocket` throws, the catch disposed the socket and raised `OnConnectionError`, and the send went ahead regardless - and `SendMessageAsync` returns a task that faults when the message could not be written, so the request that owns it is rejected at once rather than left to `RequestTimeout`. Messages are serialized whole on the socket; two concurrent messages larger than the send chunk could interleave their frames before + * `OnSessionEnded` is owed whatever wins. A `ChangeServer` or fast reconnect that a `Disconnect()` overtakes before it announced the session it retired still announces it - the retirement silenced the socket's own close callback, and nothing else knows the session. `Disconnect()` announces `UserDisconnected` itself rather than leaving it to the close callback alone: a `Connect()` issued right after it installs a new session before the old socket's close is processed, and the callback then filed the close as a stale session and said nothing. And a takeover that finds no socket takes no session either - the session belongs to whoever took the socket, and a `Connect()` after a `Disconnect()` used to announce a loss of its own for a session the disconnect was about to announce + * a fast reconnect no longer runs a second full series after the loop gave up. With `StopAfterMaxAttempts`, the loop the fast reconnect's failure started ran out of attempts, reported `Disconnected` and released its source; the fast reconnect's own wait then failed with "failed permanently", which its catch read as one more failure to retry. And a `Disconnect()` that took a handshake still in flight no longer installs a completion source nobody completes - the cancelled handshake reports no close - so the next `DisconnectAndWaitAsync` returns at once instead of waiting out its timeout + * six older defects on the same paths, found by the cold review of this change and fixed with it because the change rewrites the code they live on. `OnceOpen` reported `Connected` and started a ping timer nothing would stop after a `Disconnect()` from inside the `OnConnected` handler. `Connect()` after a `Disconnect()` ran with the intentional-disconnect flag still set, so a server that was down read as "closed permanently" and nothing reconnected - `ChangeServer` was the only path that cleared it, and the flag now follows the generation. A handshake cancelled by a takeover reported nothing, so its attempt timer went on firing `OnConnectionFailed` for the dead socket at every `ConnectionAttemptTimeout`. And `Connect()` over a socket that was closing announced no session end and swept no requests, both of which the close callback would have done had `Connect()` not retired the session underneath it + * a failure of an established connection is reported once. 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 handshake-failure callback as well as the close callback. The first announced "Initial connection failed" for a connection that had been up and in use, with an `OnDisconnect` that carried no code, and the second reported the real close. Now the close callback is the only reporter: it classifies the failure, announces the session end once and starts the reconnect. In the browser a `WebSocketException` on an open socket is classified as 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 + * a handshake this side cancelled is not reported a second time. The connect-attempt timer and a takeover cancel the socket after reporting, and in the browser the cancelled `ConnectAsync` throws `WebSocketException` ("ConnectFailure") rather than `OperationCanceledException`, which reached the connection-error callback as a second failure of the same attempt. Both found by driving the Blazor test client through a dropped connection and a connect timeout; neither is reachable from the .NET unit suite, where a cancelled handshake throws `OperationCanceledException` and a dropped connection arrives as a network error + * the documentation of `UseCheckHealth` and `InactivityTimeout` promised more than the code does: it said the health check on its own reconnects after sixty seconds without inbound data, while the inactivity check has always run only with `UseCustomPing` enabled - and deliberately so, since an idle connection with no subscriptions receives nothing by design, and silence without keepalive pings would declare a healthy socket dead every minute. The behaviour stays; the docs now say what it is. Found by driving the Blazor test client through a connection that stayed open and went silent + * pinned by tests that issue the second operation from a callback the first one runs, which lands it inside the first one's yields every time: a `Disconnect()` and a second `ChangeServer` from the session-ended handler of a `ChangeServer`, a `ChangeServer` from the `RestoringConnection` notification of the fast reconnect, a `Disconnect()` from the `OnConnected` handler, a `Connect()` after a `Disconnect()` against a server that comes up later, and a server that closes each connection the moment its handshake completes, so the reconnect loop's success and the close it has to survive arrive together + ## 11.3.2.0 06/09/2026 * **A request issued while the client is switching servers no longer hangs until `RequestTimeout`** (#177). Every path that retires a connection - `ChangeServer`, the ping-triggered fast reconnect, `Disconnect`, `DisconnectAndWaitAsync`, and the path taken when an `OnConnected` handler fails - rejected the pending requests first and cleared the socket reference afterwards. The rejection resumes the consumer, and a consumer that issues its next request from there - the second value of a page load, read from the response handler of the first - found the retired socket still installed, passed the connectivity check on it, and was written into it after the sweep that would have rejected it. Nothing completed it: the sweep had run, and a failed send is report-only. Forty seconds later it timed out, with the connection healthy for thirty-nine of them. diff --git a/Tests/Xrpl.Tests/Client/CloseAfterHandshakeServer.cs b/Tests/Xrpl.Tests/Client/CloseAfterHandshakeServer.cs new file mode 100644 index 00000000..0a1bba5f --- /dev/null +++ b/Tests/Xrpl.Tests/Client/CloseAfterHandshakeServer.cs @@ -0,0 +1,72 @@ +using System.Net.Sockets; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; + +namespace Xrpl.Tests +{ + /// + /// WebSocket server that closes its first closeFirst connections right after the + /// handshake - a close frame, then the TCP connection - and serves every connection after + /// that, answering each request with the same server_info body. + /// + /// + /// A socket that opens and closes at once is the shape of the reconnect loop's narrowest + /// window: the loop sees its attempt succeed, and the close arrives while it is deciding + /// whether it is done. Whether the client comes back from that depends on the loop and the + /// close callback agreeing on who reconnects, which is what this server is for. The shared + /// mock cannot do it - it serves every connection it accepts. + /// + internal sealed class CloseAfterHandshakeServer : WebSocketTestServerBase + { + private const string ServerInfoEnvelope = + "{\"id\":__ID__,\"status\":\"success\",\"type\":\"response\",\"result\":{\"info\":" + + "{\"build_version\":\"test-mock\",\"complete_ledgers\":\"1-1\",\"server_state\":\"full\"}}}"; + + private readonly int _closeFirst; + + private int _connections; + + public CloseAfterHandshakeServer(int closeFirst) + { + _closeFirst = closeFirst; + StartAccepting(); + } + + /// How many connections completed the handshake so far, closed ones included. + public int Connections => Volatile.Read(ref _connections); + + protected override bool ServesManyClients => true; + + protected override async Task ServeAsync(NetworkStream stream) + { + int connection = Interlocked.Increment(ref _connections); + if (connection <= _closeFirst) + { + // A close frame with no status code: FIN + opcode 0x8, empty payload. Returning + // lets the base dispose the connection behind it. + await stream.WriteAsync(new byte[] { 0x88, 0x00 }, Token).ConfigureAwait(false); + await stream.FlushAsync(Token).ConfigureAwait(false); + return; + } + + while (!Token.IsCancellationRequested) + { + string request = await ReadTextFrameAsync(stream).ConfigureAwait(false); + if (request == null) + { + return; + } + + using JsonDocument document = JsonDocument.Parse(request); + string id = document.RootElement.TryGetProperty("id", out JsonElement requestId) + ? requestId.GetRawText() + : "null"; + + byte[] response = Encoding.UTF8.GetBytes(ServerInfoEnvelope.Replace("__ID__", id)); + await WriteFragmentedMessageAsync(stream, response, fragments: 1).ConfigureAwait(false); + } + } + } +} diff --git a/Tests/Xrpl.Tests/Client/MalformedFrameServer.cs b/Tests/Xrpl.Tests/Client/MalformedFrameServer.cs new file mode 100644 index 00000000..a5c817c7 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/MalformedFrameServer.cs @@ -0,0 +1,76 @@ +using System.Net.Sockets; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; + +namespace Xrpl.Tests +{ + /// + /// WebSocket server that, on its first poisonFirst connections, answers the first + /// request with a frame the protocol forbids - a reserved opcode - and serves every + /// connection after that normally. + /// + /// + /// A forbidden frame makes the client's ReceiveAsync throw a + /// that is not a network error, on a + /// connection that was established and in use. That is the failure shape the receive loop used + /// to report twice - once as a handshake-style connection error, once as a close - and this + /// server is how a test gets one on demand. A dropped TCP connection cannot stand in for it: + /// that arrives as a network error and takes the other branch. + /// + internal sealed class MalformedFrameServer : WebSocketTestServerBase + { + private const string ServerInfoEnvelope = + "{\"id\":__ID__,\"status\":\"success\",\"type\":\"response\",\"result\":{\"info\":" + + "{\"build_version\":\"test-mock\",\"complete_ledgers\":\"1-1\",\"server_state\":\"full\"}}}"; + + private readonly int _poisonFirst; + + private int _connections; + + public MalformedFrameServer(int poisonFirst) + { + _poisonFirst = poisonFirst; + StartAccepting(); + } + + /// How many connections completed the handshake so far, poisoned ones included. + public int Connections => Volatile.Read(ref _connections); + + protected override bool ServesManyClients => true; + + protected override async Task ServeAsync(NetworkStream stream) + { + int connection = Interlocked.Increment(ref _connections); + bool poison = connection <= _poisonFirst; + + while (!Token.IsCancellationRequested) + { + string request = await ReadTextFrameAsync(stream).ConfigureAwait(false); + if (request == null) + { + return; + } + + if (poison) + { + // FIN + reserved opcode 0xB, no payload. The client's receive fails on the + // header alone; the connection is then dropped behind it. + await stream.WriteAsync(new byte[] { 0x8B, 0x00 }, Token).ConfigureAwait(false); + await stream.FlushAsync(Token).ConfigureAwait(false); + await Task.Delay(300, Token).ConfigureAwait(false); + return; + } + + using JsonDocument document = JsonDocument.Parse(request); + string id = document.RootElement.TryGetProperty("id", out JsonElement requestId) + ? requestId.GetRawText() + : "null"; + + byte[] response = Encoding.UTF8.GetBytes(ServerInfoEnvelope.Replace("__ID__", id)); + await WriteFragmentedMessageAsync(stream, response, fragments: 1).ConfigureAwait(false); + } + } + } +} diff --git a/Tests/Xrpl.Tests/Client/TestUConnectionTransitionOwner.cs b/Tests/Xrpl.Tests/Client/TestUConnectionTransitionOwner.cs new file mode 100644 index 00000000..1d415e3e --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUConnectionTransitionOwner.cs @@ -0,0 +1,891 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Net; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; + +using Xrpl.Client; +using Xrpl.Client.Exceptions; + +namespace Xrpl.Tests +{ + /// + /// Regression tests for issue #179: a transition of the connection has one owner, and an + /// operation that finds itself superseded stands down instead of overriding what came after + /// it. + /// + /// + /// + /// The four windows the issue lists are races between concurrent operations, and three of + /// them are a handful of instructions wide - not something the public API can hit on demand. + /// What it can do is issue the second operation from a callback the first one runs, which is + /// the consumer shape the issue describes and which lands the second operation inside the + /// first one's yields deterministically. Each test below is such a callback, and each asserts + /// the outcome the issue asks for: the later operation wins, the earlier one reports that it + /// lost, and nothing is done twice. + /// + /// + /// The loop-exit window (ReconnectLoopAsync against OnceClose) is exercised in + /// its reachable form: a server that closes the connection the moment the handshake completes, + /// so the close lands while the loop is deciding whether its attempt succeeded. That does not + /// open the exact gap - nothing from outside can - but it is the path through it, and a client + /// that fails to come back here is the wedge the issue describes. + /// + /// + [TestClass] + public class TestUConnectionTransitionOwner + { + private CreateMockRippled _firstRippled; + private CreateMockRippled _secondRippled; + private CreateMockRippled _thirdRippled; + private XrplClient _client; + private int _firstPort; + private int _secondPort; + private int _thirdPort; + + private static Dictionary ServerInfoResponse() => new Dictionary + { + { "type", "response" }, + { "status", "success" }, + { "result", new Dictionary + { + { "info", new Dictionary + { + { "build_version", "test-mock" }, + { "complete_ledgers", "1-1" }, + { "server_state", "full" }, + } + }, + } + }, + }; + + private static Dictionary EmptyResponse() => new Dictionary + { + { "type", "response" }, + { "status", "success" }, + { "result", new Dictionary() }, + }; + + private static CreateMockRippled StartMock(int port) + { + CreateMockRippled mock = new CreateMockRippled(port) { suppressOutput = true }; + mock.AddResponse("server_info", ServerInfoResponse()); + mock.AddResponse("ping", EmptyResponse()); + + // Start() binds, listens and hands off to BeginAccept without blocking, so the port + // is accepting when it returns - see TestUReconnectSessionRaces. + mock.Start(); + return mock; + } + + [TestInitialize] + public void MyTestInitialize() + { + _firstPort = TestUtils.GetFreePort(); + _secondPort = TestUtils.GetFreePort(); + _thirdPort = TestUtils.GetFreePort(); + _firstRippled = StartMock(_firstPort); + } + + [TestCleanup] + public async Task MyTestCleanup() + { + if (_client != null) + { + try + { + await _client.Disconnect(); + } + catch (Exception) + { + // The client may already be down; cleanup must not mask the test result. + } + + _client = null; + } + + _firstRippled?.Stop(); + _secondRippled?.Stop(); + _thirdRippled?.Stop(); + } + + private static XrplClient.ClientOptions Options() => new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + ReconnectBaseDelay = TimeSpan.FromMilliseconds(50), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(400), + MaxReconnectAttempts = 100, + StopAfterMaxAttempts = false, + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + UseCustomPing = false, + }; + + /// + /// The health check hands a silent connection to the fast-reconnect path in well under a + /// second with these - the same knobs TestUFastReconnectSettling uses. + /// + private static XrplClient.ClientOptions FastReconnectOptions() => new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(500), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(5), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(3), + UseCustomPing = true, + HealthCheckInterval = TimeSpan.FromMilliseconds(200), + InactivityTimeout = TimeSpan.FromMilliseconds(500), + }; + + private static string UrlOf(int port) => $"ws://127.0.0.1:{port}"; + + private static async Task WaitUntilAsync(Func condition, TimeSpan timeout, string what) + { + Stopwatch clock = Stopwatch.StartNew(); + while (!condition()) + { + Assert.IsTrue(clock.Elapsed < timeout, $"Timed out after {timeout.TotalSeconds:F0}s waiting for: {what}"); + await Task.Delay(50); + } + } + + /// + /// Window 4 of the issue. ChangeServer yields several times before it connects; a + /// Disconnect() landing in one of those yields used to be overridden - the switch + /// reset the disconnect and connected, and the client was online after the consumer had + /// taken it down. Issued from the session-ended handler, the disconnect lands in exactly + /// that yield every time. + /// + [TestMethod] + public async Task TestDisconnectFromSessionEndedHandlerWinsOverChangeServer() + { + _secondRippled = StartMock(_secondPort); + _client = new XrplClient(UrlOf(_firstPort), Options()); + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the first server."); + + int connectedAfterSwitch = 0; + bool switching = false; + + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged) + { + await _client.Disconnect(); + } + }; + _client.OnConnected += () => + { + if (Volatile.Read(ref switching)) + { + Interlocked.Increment(ref connectedAfterSwitch); + } + + return Task.CompletedTask; + }; + + Volatile.Write(ref switching, true); + Exception failure = null; + try + { + await _client.connection.ChangeServer(UrlOf(_secondPort)); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType( + failure, + $"A ChangeServer that a Disconnect() overtook must say the client is disconnected, not {failure?.GetType().Name ?? "return normally"}."); + + // Room for the connection nobody should be making. + await Task.Delay(TimeSpan.FromSeconds(1)); + + Assert.IsFalse(_client.connection.IsConnected(), "The client is online after the consumer disconnected it: ChangeServer overrode the Disconnect()."); + Assert.IsNull(_client.connection.ws, "A socket is installed after the consumer disconnected the client."); + Assert.AreEqual(XrpConnectionState.Disconnected, _client.connection.CurrentConnectionState); + Assert.AreEqual(0, connectedAfterSwitch, "OnConnected was raised for a switch the consumer cancelled with Disconnect()."); + } + + /// + /// Two switches, the second issued from the first one's session-ended handler. The first + /// used to return success after the second had connected elsewhere - and, worse, wrote its + /// own target into the url afterwards, so the client reported a server it was not on. + /// + [TestMethod] + public async Task TestLaterChangeServerFromSessionEndedHandlerSupersedesTheEarlierOne() + { + _secondRippled = StartMock(_secondPort); + _thirdRippled = StartMock(_thirdPort); + _client = new XrplClient(UrlOf(_firstPort), Options()); + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the first server."); + + int connectedAfterSwitch = 0; + int nested = 0; + bool switching = false; + Exception nestedFailure = null; + + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && Interlocked.Exchange(ref nested, 1) == 0) + { + try + { + await _client.connection.ChangeServer(UrlOf(_thirdPort)); + } + catch (Exception error) + { + nestedFailure = error; + } + } + }; + _client.OnConnected += () => + { + if (Volatile.Read(ref switching)) + { + Interlocked.Increment(ref connectedAfterSwitch); + } + + return Task.CompletedTask; + }; + + Volatile.Write(ref switching, true); + Exception failure = null; + try + { + await _client.connection.ChangeServer(UrlOf(_secondPort)); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsNull(nestedFailure, $"The later switch is the one that should have won, but it failed: {nestedFailure}"); + Assert.IsInstanceOfType( + failure, + $"A ChangeServer that a later ChangeServer overtook must report that it was superseded, not {failure?.GetType().Name ?? "return normally"}."); + + await Task.Delay(TimeSpan.FromSeconds(1)); + + Assert.IsTrue(_client.connection.IsConnected(), "The client must be connected after the later switch."); + Assert.AreEqual(UrlOf(_thirdPort), _client.connection.GetUrl(), "The client reports a server other than the one it is connected to."); + Assert.AreEqual(1, connectedAfterSwitch, "The client connected more than once for two switches of which only the later one should have connected."); + } + + /// + /// Window 3 of the issue, in its reachable form. A status handler that answers + /// RestoringConnection with a switch to another server starts a transition of its + /// own from inside the fast reconnect. The fast reconnect used to carry on regardless - + /// its attempt was cancelled underneath it, it read that as a failure and started a + /// reconnect loop, reporting "Reconnection failed" for a switch that was under way. Now + /// it stands down at the check that follows the callback. + /// + [TestMethod] + public async Task TestChangeServerFromRestoringConnectionHandlerSupersedesTheFastReconnect() + { + using SilentOnPingServer silentServer = new SilentOnPingServer(); + _secondRippled = StartMock(_secondPort); + _client = new XrplClient(silentServer.Url, FastReconnectOptions()); + + object gate = new object(); + bool switched = false; + int connectedAfterSwitch = 0; + List reconnectFailures = new List(); + List restoringAfterConnected = new List(); + + _client.OnConnectionStatus += info => + { + lock (gate) + { + if (info.ConnectionState == XrpConnectionState.RestoringConnection && !switched) + { + // The health check just handed the silent connection to the fast-reconnect + // path. Move the client elsewhere from inside its own notification. + switched = true; + _ = _client.connection.ChangeServer(UrlOf(_secondPort)); + return; + } + + if (info.ConnectionState == XrpConnectionState.RestoringConnection && switched) + { + if (info.Message.StartsWith("Reconnection failed", StringComparison.Ordinal)) + { + reconnectFailures.Add(info.Message); + } + + if (connectedAfterSwitch > 0) + { + restoringAfterConnected.Add(info.Message); + } + } + + if (info.ConnectionState == XrpConnectionState.Connected && switched) + { + connectedAfterSwitch++; + } + } + }; + + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the silent server."); + + await WaitUntilAsync( + () => { lock (gate) { return connectedAfterSwitch > 0; } }, + TimeSpan.FromSeconds(15), + "the switch issued from the RestoringConnection handler to connect"); + + // Long enough for a reconnect loop the fast reconnect should not have started to + // report itself. + await Task.Delay(TimeSpan.FromSeconds(3)); + + lock (gate) + { + Assert.AreEqual( + 0, + reconnectFailures.Count, + "The fast reconnect reported a failure of its own attempt after a switch from its status handler had taken the connection over: " + string.Join(" | ", reconnectFailures)); + Assert.AreEqual( + 0, + restoringAfterConnected.Count, + "RestoringConnection was reported on a client the switch had connected: " + string.Join(" | ", restoringAfterConnected)); + Assert.AreEqual(1, connectedAfterSwitch, "The client connected more than once after the switch."); + } + + Assert.IsTrue(_client.connection.IsConnected(), "The client must be connected to the server the handler switched to."); + Assert.AreEqual(UrlOf(_secondPort), _client.connection.GetUrl()); + } + + /// + /// Window 1 of the issue, in its reachable form: the reconnect loop's attempt succeeds and + /// the connection closes at once, so the close is processed while the loop is deciding + /// whether it is done. The loop and the close callback have to agree on who reconnects; + /// if neither does, the client stays down with a server that is up. + /// + [TestMethod] + public async Task TestReconnectLoopSurvivesACloseRightAfterItsAttemptOpened() + { + using CloseAfterHandshakeServer server = new CloseAfterHandshakeServer(closeFirst: 3); + _client = new XrplClient(server.Url, Options()); + + // Straight to the connection: the client's Connect() adds a server_info read, and the + // closing connections would have that failing for reasons that are not this test's. + await _client.connection.Connect(CancellationToken.None); + + // Connect() returns as soon as a socket is open, which can be before the server's close + // frame for it has even arrived - so the outcome is waited for, not asserted at once: + // the client must end up connected on a connection the server did not close. + await WaitUntilAsync( + () => server.Connections >= 4 && _client.connection.IsConnected(), + TimeSpan.FromSeconds(15), + $"the client to reach the server after it stopped closing connections (state: {_client.connection.CurrentConnectionState}, connections seen: {server.Connections})"); + + Dictionary response = await _client.connection + .Request(new Dictionary { { "command", "server_info" } }) + .Typed(); + Assert.IsNotNull(response, "The client must be usable on the connection the loop ended on."); + } + + /// + /// A Disconnect() issued while a handshake is pending must leave nothing behind: not + /// the socket that was connecting, and not a connection it would have made. + /// + [TestMethod] + public async Task TestDisconnectDuringAPendingHandshakeLeavesNoSocketBehind() + { + // Accepts TCP and never answers the upgrade: the handshake stays pending. + using TcpListener silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + int port = ((IPEndPoint)silent.LocalEndpoint).Port; + + _client = new XrplClient(UrlOf(port), Options()); + + Task connecting = _client.connection.Connect(CancellationToken.None); + await Task.Delay(TimeSpan.FromMilliseconds(300)); + Assert.IsFalse(connecting.IsCompleted, "Precondition: the handshake must still be pending."); + + await _client.Disconnect(); + + Exception failure = null; + try + { + await connecting; + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType( + failure, + $"A Connect() that a Disconnect() overtook must say the client is disconnected, not {failure?.GetType().Name ?? "return normally"}."); + Assert.IsNull(_client.connection.ws, "The socket that was connecting is still installed after Disconnect()."); + Assert.AreEqual(XrpConnectionState.Disconnected, _client.connection.CurrentConnectionState); + } + + /// + /// A Disconnect() from inside the OnConnected handler must be the last word: + /// the connect path that ran the handler used to report Connected on top of it and + /// start a ping timer nothing would ever stop. + /// + [TestMethod] + public async Task TestDisconnectFromConnectedHandlerIsNotOverriddenByConnected() + { + _client = new XrplClient(UrlOf(_firstPort), Options()); + + object gate = new object(); + List statesAfterDisconnect = new List(); + bool disconnected = false; + + _client.OnConnectionStatus += info => + { + lock (gate) + { + if (disconnected) + { + statesAfterDisconnect.Add(info.ConnectionState); + } + } + }; + _client.OnConnected += async () => + { + await _client.Disconnect(); + lock (gate) + { + disconnected = true; + } + }; + + try + { + await _client.connection.Connect(CancellationToken.None); + } + catch (Exception) + { + // Expected: the handler disconnected the client it was connecting. + } + + await Task.Delay(TimeSpan.FromMilliseconds(500)); + + Assert.IsFalse(_client.connection.IsConnected(), "The client is connected after its OnConnected handler disconnected it."); + Assert.AreEqual(XrpConnectionState.Disconnected, _client.connection.CurrentConnectionState, "The connect path reported Connected over the handler's Disconnect()."); + Assert.IsFalse(_client.connection.IsPingTimerRunning, "A ping timer was started for a connection the handler had already taken down."); + lock (gate) + { + Assert.IsFalse( + statesAfterDisconnect.Contains(XrpConnectionState.Connected), + "Connected was reported after the handler's Disconnect(): " + string.Join(", ", statesAfterDisconnect)); + } + } + + /// + /// Connect() after a user Disconnect(), to a server that is down, must leave + /// the client reconnecting. The intentional-disconnect flag the disconnect left behind + /// used to make the failed handshake look like a user disconnect - "closed permanently", + /// no loop - and ChangeServer was the only path that cleared it. + /// + [TestMethod] + public async Task TestConnectAfterUserDisconnectReconnectsWhenTheServerComesUp() + { + XrplClient.ClientOptions options = Options(); + options.ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(3); + options.ConnectionAttemptTimeout = TimeSpan.FromSeconds(3); + + // A client pointed at a port where nothing listens yet. Disconnect() on a client that + // never connected still records a user disconnect, which is all the flag needs. + _client = new XrplClient(UrlOf(_secondPort), options); + await _client.Disconnect(); + + // The attempt fails, and the client must be left reconnecting, not "closed permanently". + try + { + await _client.connection.Connect(CancellationToken.None); + } + catch (Exception) + { + // Expected - nothing is listening there yet. + } + + Assert.AreNotEqual( + XrpConnectionState.Disconnected, + _client.connection.CurrentConnectionState, + "A server that is down is a connection failure, not a permanent disconnect - the flag of the earlier Disconnect() was read as this connection's."); + + Assert.IsTrue( + TestUtils.IsPortStillFree(_secondPort), + $"Port {_secondPort} was taken by another process while the test held it — rerun."); + _secondRippled = StartMock(_secondPort); + + await WaitUntilAsync( + () => _client.connection.IsConnected(), + TimeSpan.FromSeconds(30), + "the client to reconnect after the server came up"); + } + + /// + /// A ChangeServer that a Disconnect() overtakes before the switch is announced + /// still owes the consumer the end of the session it retired: the retirement silenced the + /// socket's own close callback, and the disconnect does not know the session. + /// + [TestMethod] + public async Task TestChangeServerSupersededBeforeAnnouncingStillAnnouncesTheSessionEnd() + { + _secondRippled = StartMock(_secondPort); + _client = new XrplClient(UrlOf(_firstPort), Options()); + await _client.Connect(); + + object gate = new object(); + List ended = new List(); + bool disconnecting = false; + + _client.OnSessionEnded += (reason, _) => + { + lock (gate) + { + ended.Add(reason); + } + + return Task.CompletedTask; + }; + _client.OnConnectionStatus += info => + { + // The first thing ChangeServer reports, before it announces the session end. A + // Disconnect() from here takes over before the announcement. + if (info.ConnectionState == XrpConnectionState.Connecting && + info.Message.StartsWith("ChangeServer", StringComparison.Ordinal) && + !disconnecting) + { + disconnecting = true; + _ = _client.Disconnect(); + } + }; + + Exception failure = null; + try + { + await _client.connection.ChangeServer(UrlOf(_secondPort)); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType(failure, $"Expected NotConnectedException, got {failure?.GetType().Name ?? "no exception"}."); + + await Task.Delay(TimeSpan.FromSeconds(1)); + + lock (gate) + { + Assert.AreEqual(1, ended.Count, "OnSessionEnded must be raised exactly once for the session the switch retired: " + string.Join(", ", ended)); + Assert.AreEqual(SessionEndReason.ServerChanged, ended[0]); + } + + Assert.IsFalse(_client.connection.IsConnected(), "The Disconnect() issued from the status handler must win."); + } + + /// + /// A fast reconnect whose loop ran out of attempts must not start a second series: the + /// loop reported Disconnected and released its source, and the fast reconnect's own + /// wait ends with the "failed permanently" refusal. Read as a failure to retry, that + /// refusal used to buy another full run of attempts, and StopAfterMaxAttempts meant + /// nothing. + /// + [TestMethod] + public async Task TestFastReconnectDoesNotRestartAfterTheLoopGaveUp() + { + SilentOnPingServer silentServer = new SilentOnPingServer(); + try + { + XrplClient.ClientOptions options = FastReconnectOptions(); + options.StopAfterMaxAttempts = true; + options.MaxReconnectAttempts = 2; + options.ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(20); + + _client = new XrplClient(silentServer.Url, options); + + object gate = new object(); + bool retired = false; + int stoppedReports = 0; + List restoringAfterStopped = new List(); + + _client.OnConnectionStatus += info => + { + lock (gate) + { + if (info.ConnectionState == XrpConnectionState.RestoringConnection && !retired) + { + // The health check handed the silent connection to the fast reconnect; + // take the server down so every attempt fails at the socket. + retired = true; + silentServer.Dispose(); + return; + } + + if (info.ConnectionState == XrpConnectionState.Disconnected && + info.Message.StartsWith("Reconnection stopped", StringComparison.Ordinal)) + { + stoppedReports++; + return; + } + + if (info.ConnectionState == XrpConnectionState.RestoringConnection && stoppedReports > 0) + { + restoringAfterStopped.Add(info.Message); + } + } + }; + + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the silent server."); + + await WaitUntilAsync( + () => { lock (gate) { return stoppedReports > 0; } }, + TimeSpan.FromSeconds(20), + "the reconnect loop to give up after MaxReconnectAttempts"); + + // Room for a second series to report itself, had one been started. + await Task.Delay(TimeSpan.FromSeconds(3)); + + lock (gate) + { + Assert.AreEqual( + 0, + restoringAfterStopped.Count, + "RestoringConnection was reported after the loop had given up: " + string.Join(" | ", restoringAfterStopped)); + Assert.AreEqual(1, stoppedReports, "The loop gave up more than once - a second series ran."); + } + + Assert.AreEqual(XrpConnectionState.Disconnected, _client.connection.CurrentConnectionState); + } + finally + { + silentServer.Dispose(); + } + } + + /// + /// Disconnect() then Connect() at once - the ordinary way to bounce a client. + /// The disconnect closes its socket in the background, and the close callback is what + /// announces ; a Connect() that + /// retired that session underneath would silence the callback and announce a loss of its + /// own instead. + /// + [TestMethod] + public async Task TestConnectRightAfterDisconnectKeepsTheUserDisconnectAnnouncement() + { + _client = new XrplClient(UrlOf(_firstPort), Options()); + await _client.Connect(); + + object gate = new object(); + List ended = new List(); + + _client.OnSessionEnded += (reason, _) => + { + lock (gate) + { + ended.Add(reason); + } + + return Task.CompletedTask; + }; + + await _client.Disconnect(); + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "The client must be connected again."); + + await Task.Delay(TimeSpan.FromSeconds(1)); + + lock (gate) + { + Assert.AreEqual(1, ended.Count, "The session must end exactly once: " + string.Join(", ", ended)); + Assert.AreEqual(SessionEndReason.UserDisconnected, ended[0], "The session ended because the consumer disconnected, and the announcement must say so."); + } + } + + /// + /// After a Disconnect() that took a handshake still in flight, a + /// DisconnectAndWaitAsync must return at once: the cancelled handshake reports no + /// close, so there is nothing to wait for - and a completion source installed for it would + /// never complete. + /// + [TestMethod] + public async Task TestDisconnectAndWaitAfterADisconnectDuringHandshakeReturnsAtOnce() + { + using TcpListener silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + int port = ((IPEndPoint)silent.LocalEndpoint).Port; + + _client = new XrplClient(UrlOf(port), Options()); + + Task connecting = _client.connection.Connect(CancellationToken.None); + await Task.Delay(TimeSpan.FromMilliseconds(300)); + await _client.Disconnect(); + try + { + await connecting; + } + catch (Exception) + { + // Expected: the handshake was cancelled by the disconnect. + } + + Stopwatch clock = Stopwatch.StartNew(); + await _client.DisconnectAndWaitAsync(TimeSpan.FromSeconds(5)); + clock.Stop(); + + Assert.IsTrue( + clock.Elapsed < TimeSpan.FromSeconds(2), + $"DisconnectAndWaitAsync on a disconnected client waited {clock.Elapsed.TotalSeconds:F1}s - on a completion source nobody completes."); + } + + /// + /// A failure of an established connection that is not a network error - here a frame the + /// protocol forbids - is one event and must be reported once: one OnDisconnect, one + /// OnSessionEnded, a RestoringConnection that says the connection was lost. + /// The receive loop used to route it through the handshake-failure callback as well as + /// the close callback, which announced "Initial connection failed" for a connection that + /// had been up and in use, and a second OnDisconnect with no code behind it. + /// + [TestMethod] + public async Task TestFailureOfAnEstablishedConnectionIsReportedOnce() + { + using MalformedFrameServer server = new MalformedFrameServer(poisonFirst: 1); + _client = new XrplClient(server.Url, Options()); + + object gate = new object(); + int disconnects = 0; + List ended = new List(); + List statuses = new List(); + int connectedAfterFailure = 0; + + _client.OnDisconnect += (_, _) => + { + Interlocked.Increment(ref disconnects); + return Task.CompletedTask; + }; + _client.OnSessionEnded += (reason, _) => + { + lock (gate) + { + ended.Add(reason); + } + + return Task.CompletedTask; + }; + _client.OnConnectionStatus += info => + { + lock (gate) + { + statuses.Add($"[{info.ConnectionState}] {info.Message}"); + if (info.ConnectionState == XrpConnectionState.Connected && ended.Count > 0) + { + connectedAfterFailure++; + } + } + }; + + await _client.connection.Connect(CancellationToken.None); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the poisoned server."); + + // The first request is answered with the forbidden frame: the receive loop fails on a + // connection that is up and in use. + Exception failure = null; + try + { + await _client.connection.Request( + new Dictionary { { "command", "server_info" } }, + timeout: TimeSpan.FromSeconds(10)); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsNotNull(failure, "The request written to the connection that failed must be rejected."); + Assert.IsNotInstanceOfType( + failure, + "The request waited out its timeout instead of being rejected when the connection failed."); + + await WaitUntilAsync( + () => { lock (gate) { return connectedAfterFailure > 0; } }, + TimeSpan.FromSeconds(15), + "the reconnect loop to bring the client back on a healthy connection"); + + // Room for a second report to arrive, had one been made. + await Task.Delay(TimeSpan.FromMilliseconds(500)); + + lock (gate) + { + Assert.AreEqual(1, Volatile.Read(ref disconnects), "OnDisconnect must be raised once for one failed connection. Statuses: " + string.Join(" | ", statuses)); + Assert.AreEqual(1, ended.Count, "OnSessionEnded must be raised once: " + string.Join(", ", ended)); + Assert.AreEqual(SessionEndReason.ConnectionLost, ended[0]); + Assert.IsFalse( + statuses.Any(s => s.Contains("Initial connection failed", StringComparison.Ordinal)), + "A connection that was up and in use was reported as a failed initial connection: " + string.Join(" | ", statuses)); + Assert.IsTrue( + statuses.Any(s => s.StartsWith("[RestoringConnection]", StringComparison.Ordinal)), + "The failure must be reported as RestoringConnection: " + string.Join(" | ", statuses)); + } + + Dictionary response = await _client.connection + .Request(new Dictionary { { "command", "server_info" } }) + .Typed(); + Assert.IsNotNull(response, "The client must be usable on the connection the loop brought up."); + } + + /// + /// Item 5 of the issue. Under a request + /// refused for want of a connection used to carry the runtime's default message, which a + /// consumer classifying failures by text could not recognise. + /// + [TestMethod] + public async Task TestImmediateFailRefusalSaysTheClientIsNotConnected() + { + XrplClient.ClientOptions options = Options(); + options.ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(2); + options.ConnectionAttemptTimeout = TimeSpan.FromSeconds(2); + + _client = new XrplClient(UrlOf(_firstPort), options); + await _client.Connect(); + + // A switch to a port nobody listens on leaves the client reconnecting - the state in + // which a request reaches the policy at all. + try + { + await _client.connection.ChangeServer(UrlOf(_secondPort)); + } + catch (Exception) + { + // Expected: nothing is listening there. + } + + Exception failure = null; + try + { + await _client.connection.Request(new Dictionary { { "command", "server_info" } }); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType(failure, $"Expected NotConnectedException, got {failure?.GetType().Name ?? "no exception"}."); + StringAssert.Contains(failure.Message, "not connected", StringComparison.OrdinalIgnoreCase); + Assert.IsFalse( + failure.Message.Contains("Exception of type", StringComparison.Ordinal), + "The refusal carries the runtime's default message instead of saying what happened."); + + Assert.AreEqual( + NotConnectedException.DefaultMessage, + new NotConnectedException().Message, + "A NotConnectedException built without a message must still say what it is."); + } + } +} diff --git a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs index 56a8e4ed..903b3595 100644 --- a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs +++ b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs @@ -11,8 +11,8 @@ namespace Xrpl.Tests { /// /// Concurrency smoke tests for the reconnect session — the _reconnectCts / - /// _reconnectLoop / _reconnectAttempts triple, which is now updated under a - /// shared lock. + /// _reconnectLoopGeneration / _reconnectAttempts triple, which is updated under + /// the transition lock together with the generation that owns the connection (issue #179). /// /// /// diff --git a/Tests/Xrpl.Tests/Client/TestUWebSocketClientSend.cs b/Tests/Xrpl.Tests/Client/TestUWebSocketClientSend.cs new file mode 100644 index 00000000..2026886d --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUWebSocketClientSend.cs @@ -0,0 +1,123 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +using Xrpl.Client; + +namespace Xrpl.Tests +{ + /// + /// Item 6 of issue #179: a send never reconnects, a send on a socket that is not open fails, + /// and the failure is observable. + /// + /// + /// SendMessageAsync used to answer a socket that was not Open with + /// Connect() - ConnectAsync on an already used ClientWebSocket, which + /// throws, disposes the socket and raises OnConnectionError - and then went on to send + /// regardless. The connection layer read that error as a failure of the connection. + /// + [TestClass] + public class TestUWebSocketClientSend + { + /// + /// A socket the server closed is not open. Sending on it must report a send failure and + /// nothing else - no connection error, no disposal. + /// + [TestMethod] + public async Task TestSendOnAClosedSocketDoesNotReconnectIt() + { + using CloseAfterHandshakeServer server = new CloseAfterHandshakeServer(closeFirst: 1); + WebSocketClient client = WebSocketClient.Create(server.Url); + + int connectionErrors = 0; + int sendErrors = 0; + TaskCompletionSource closed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + client.OnConnectionError((_, _) => + { + Interlocked.Increment(ref connectionErrors); + return Task.CompletedTask; + }); + client.OnError((_, _) => + { + Interlocked.Increment(ref sendErrors); + return Task.CompletedTask; + }); + client.OnDisconnect((_, _, _) => + { + closed.TrySetResult(true); + return Task.CompletedTask; + }); + + await client.Connect(); + + Task finished = await Task.WhenAny(closed.Task, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.AreSame(closed.Task, finished, "Precondition: the server must have closed the connection."); + Assert.AreNotEqual(System.Net.WebSockets.WebSocketState.Open, client.State, "Precondition: the socket must not be open."); + + client.SendMessage("{\"command\":\"ping\"}"); + await Task.Delay(TimeSpan.FromMilliseconds(500)); + + Assert.AreEqual( + 0, + connectionErrors, + "A send on a socket that is not open tried to reconnect it: ConnectAsync on a used socket threw, and the throw was reported as a connection error."); + Assert.IsFalse(client.IsDisposed, "The send disposed the socket."); + Assert.AreEqual(1, sendErrors, "The failed send must be reported through the error callback, once."); + + Exception failure = null; + try + { + await client.SendMessageAsync(Encoding.UTF8.GetBytes("{\"command\":\"ping\"}")); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType( + failure, + $"A send on a socket that is not open must fault, not {failure?.GetType().Name ?? "complete"}."); + + client.Dispose(); + } + + /// + /// A socket that never connected has nothing to send into. The task says so; the + /// fire-and-forget entry point reports it and does not throw. + /// + [TestMethod] + public async Task TestSendOnASocketThatNeverConnectedFaults() + { + WebSocketClient client = WebSocketClient.Create("ws://127.0.0.1:1/"); + + int sendErrors = 0; + client.OnError((_, _) => + { + Interlocked.Increment(ref sendErrors); + return Task.CompletedTask; + }); + + Exception failure = null; + try + { + await client.SendMessageAsync(Encoding.UTF8.GetBytes("x")); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsInstanceOfType(failure, $"Expected InvalidOperationException, got {failure?.GetType().Name ?? "no exception"}."); + + client.SendMessage("x"); + await Task.Delay(TimeSpan.FromMilliseconds(200)); + Assert.AreEqual(1, sendErrors, "The fire-and-forget send must report its failure through the error callback."); + + client.Dispose(); + } + } +} diff --git a/Tests/Xrpl.Tests/Client/WebSocketTestServerBase.cs b/Tests/Xrpl.Tests/Client/WebSocketTestServerBase.cs index baea79c7..4128866d 100644 --- a/Tests/Xrpl.Tests/Client/WebSocketTestServerBase.cs +++ b/Tests/Xrpl.Tests/Client/WebSocketTestServerBase.cs @@ -54,6 +54,14 @@ protected void StartAccepting() /// Serves one connected client; the handshake has already completed. protected abstract Task ServeAsync(NetworkStream stream); + /// + /// Whether the server keeps accepting after its first client. The default serves one + /// client and holds that connection open until the server is disposed; a server that + /// answers serves clients one after another, and each connection + /// ends when returns for it. + /// + protected virtual bool ServesManyClients => false; + /// /// The exception that ended the accept loop, if it ended badly. Recorded rather than /// swallowed so a broken server shows up as itself instead of as the caller's timeout. @@ -75,20 +83,27 @@ private async Task AcceptAsync() { try { - using TcpClient client = await _listener.AcceptTcpClientAsync(Token).ConfigureAwait(false); - client.NoDelay = true; - NetworkStream stream = client.GetStream(); - - string request = await ReadUntilHeadersEndAsync(stream).ConfigureAwait(false); - string key = Helpers.GetHandshakeRequestKey(request); - byte[] response = Encoding.ASCII.GetBytes(Helpers.GetHandshakeResponse(Helpers.HashKey(key))); - await stream.WriteAsync(response, Token).ConfigureAwait(false); - await stream.FlushAsync(Token).ConfigureAwait(false); + do + { + using TcpClient client = await _listener.AcceptTcpClientAsync(Token).ConfigureAwait(false); + client.NoDelay = true; + NetworkStream stream = client.GetStream(); - await ServeAsync(stream).ConfigureAwait(false); + string request = await ReadUntilHeadersEndAsync(stream).ConfigureAwait(false); + string key = Helpers.GetHandshakeRequestKey(request); + byte[] response = Encoding.ASCII.GetBytes(Helpers.GetHandshakeResponse(Helpers.HashKey(key))); + await stream.WriteAsync(response, Token).ConfigureAwait(false); + await stream.FlushAsync(Token).ConfigureAwait(false); - // Hold the connection open until the test disposes the server. - await Task.Delay(Timeout.InfiniteTimeSpan, Token).ConfigureAwait(false); + await ServeAsync(stream).ConfigureAwait(false); + + if (!ServesManyClients) + { + // Hold the connection open until the test disposes the server. + await Task.Delay(Timeout.InfiniteTimeSpan, Token).ConfigureAwait(false); + } + } + while (ServesManyClients && !Token.IsCancellationRequested); } catch (OperationCanceledException) { diff --git a/Xrpl/Client/ConnectionSession.cs b/Xrpl/Client/ConnectionSession.cs index 1ac24d81..4d071d59 100644 --- a/Xrpl/Client/ConnectionSession.cs +++ b/Xrpl/Client/ConnectionSession.cs @@ -21,6 +21,19 @@ internal class ConnectionSession private static long _sessionIdCounter = 0; public long SessionId { get; } + + /// + /// The connection transition this session was created under - see + /// Connection.TakeOver. + /// + /// + /// A socket callback carries the session, and the session carries the generation, so a + /// close or a failure can tell whether the transition that opened this socket is still the + /// one in charge of the connection. If it is not, a later operation owns the connection now + /// and the callback confines itself to announcing that the session ended. + /// + public long Generation { get; } + public WebSocketClient? Socket { get; private set; } public bool IsIntentionalDisconnect { get; set; } public bool IsRetiring { get; private set; } @@ -55,9 +68,10 @@ public void MarkAsOpened() /// public Task Completion => _completionTcs.Task; - public ConnectionSession(WebSocketClient socket) + public ConnectionSession(WebSocketClient socket, long generation) { SessionId = Interlocked.Increment(ref _sessionIdCounter); + Generation = generation; Socket = socket; IsIntentionalDisconnect = false; IsRetiring = false; diff --git a/Xrpl/Client/Exceptions/XrplException.cs b/Xrpl/Client/Exceptions/XrplException.cs index 554c9660..51f274e2 100644 --- a/Xrpl/Client/Exceptions/XrplException.cs +++ b/Xrpl/Client/Exceptions/XrplException.cs @@ -78,7 +78,15 @@ public ConnectionException(string message) : base(message) /// public class NotConnectedException : XrplException { - public NotConnectedException(string message = null) : base(message) + /// + /// The message used when none is given. A bare throw new NotConnectedException() + /// used to carry the runtime's "Exception of type ... was thrown", which says nothing to a + /// consumer that classifies failures by their text. + /// + public const string DefaultMessage = + "The client is not connected to a server. Call Connect() first, or wait for the connection to be restored."; + + public NotConnectedException(string message = null) : base(message ?? DefaultMessage) { } } @@ -90,6 +98,10 @@ public class DisconnectedException : XrplException public DisconnectedException(string message) : base(message) { } + + public DisconnectedException(string message, Exception? innerException) : base(message, innerException) + { + } } /// /// Exception thrown when rippled is not initialized. diff --git a/Xrpl/Client/IXrplClient.cs b/Xrpl/Client/IXrplClient.cs index aa968dec..0b34a01b 100644 --- a/Xrpl/Client/IXrplClient.cs +++ b/Xrpl/Client/IXrplClient.cs @@ -260,9 +260,12 @@ public void SetNetworkId(uint? networkId) /// /// /// Throws when the client gave up - a - /// handler that fails every time, or a server that never comes up. - /// means what it says and nothing else: the - /// caller's own was cancelled. + /// handler that fails every time, or a server that never comes up - and when a + /// issued while this call was connecting won: the client is down, + /// and this call did not connect it. means what + /// it says and nothing else: the caller's own was + /// cancelled. A ChangeServer issued meanwhile does not fail this call - it returns + /// once the client is connected, wherever that switch took it. /// /// /// Cancels the wait for a connection. @@ -685,6 +688,30 @@ public void SetNetworkId(uint? networkId) Task Autofill(T tx, int? signersCount = null, CancellationToken cancellationToken = default) where T : ITransactionRequest; Task GetLedgerIndex(CancellationToken cancellationToken = default); Task GetXrpBalance(string address, CancellationToken cancellationToken = default); + + /// + /// Moves the client to another server: retires the current session, connects to + /// and reads its network id. + /// + /// + /// + /// The switch is announced through with + /// before the new connection is opened; the + /// subscriptions held against the old connection do not follow the client. + /// + /// + /// A later , or ChangeServer - from + /// another thread, or from a handler this call runs - takes the connection over, and this + /// call stops where it is: when a + /// won, because the client is down; + /// when anything else won, because the client is + /// connecting, or connected, somewhere this call did not ask for. The old session is + /// retired either way. + /// + /// + /// The WebSocket URL of the server to move to. + /// Options for the new connection; null keeps the current ones. + /// Cancels the wait for the new connection. Task ChangeServer(string server, ClientOptions? options = null, CancellationToken cancellationToken = default); string EnsureClassicAddress(string address); diff --git a/Xrpl/Client/WebSocketClient.cs b/Xrpl/Client/WebSocketClient.cs index ca59f487..8c957e30 100644 --- a/Xrpl/Client/WebSocketClient.cs +++ b/Xrpl/Client/WebSocketClient.cs @@ -34,6 +34,12 @@ public class WebSocketClient : IDisposable private readonly CancellationToken _cancellationToken; private Task? _receiveTask; private readonly SemaphoreSlim _disconnectLock = new SemaphoreSlim(1, 1); + + // One message at a time. ClientWebSocket serializes frames, not messages: two messages + // larger than SendChunkSize sent concurrently could interleave their frames. The lock + // also gives a send that queued behind another one a place to notice that the socket was + // retired in the meantime - see SendMessageAsync. + private readonly SemaphoreSlim _sendLock = new SemaphoreSlim(1, 1); private volatile bool _isIntentionalDisconnect; public SocketFailureReason FailureReason { get; private set; } = SocketFailureReason.None; @@ -257,65 +263,99 @@ internal WebSocketClient OnMessageReceived(Func o } /// - /// Send a UTF8 string to the WebSocket server. + /// Sends a UTF-8 string to the WebSocket server, fire-and-forget. /// + /// + /// A failure is reported through the error callback and nowhere else - this is the + /// keepalive's entry point, and a consumer's, and neither has a request to reject. A + /// caller that does have one uses and observes the + /// task. Neither entry point ever reconnects: a send on a socket that is not open fails, + /// it does not call . It used to - ConnectAsync on an already + /// used throws, the catch disposed the socket and raised + /// OnConnectionError, and the send went ahead regardless. + /// /// The message to send public void SendMessage(string message) { - SendMessageAsync(Encoding.UTF8.GetBytes(message)); + _ = ReportSendFailureAsync(SendMessageAsync(Encoding.UTF8.GetBytes(message))); + } + + private async Task ReportSendFailureAsync(Task send) + { + try + { + await send.ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // The socket was cancelled underneath the send - a close the owner asked for. + } + catch (Exception e) + { + Debug.WriteLine($"{DateTime.Now}WebSocket send failed: {e.GetType().Name}: {e.Message}"); + CallOnError(e); + } } /// - /// Send a byte array to the WebSocket server. + /// Sends a byte array to the WebSocket server. The returned task faults if the message + /// could not be written, so the owner of a request can reject it instead of leaving it to + /// its timeout. /// + /// + /// + /// The write is issued synchronously when the send lock is free: everything up to the + /// first incomplete await of + /// runs on the caller's thread. Connection relies on that - it starts the send + /// under the lock its retirement paths take, so a retirement either finds the request not + /// yet sent, or already handed to the socket. + /// + /// + /// A send that queued behind another one re-checks the socket once it holds the lock: the + /// retirement marks the socket before it lets go of it, and that mark is what refuses a + /// message that would otherwise reach a server the client has left. Between that check and + /// the write there is no lock - a few instructions - and that is the residue this method + /// does not close. + /// + /// /// The data to send - private async void SendMessageAsync(byte[] message) + /// The socket is not open. + public async Task SendMessageAsync(byte[] message) { - if (_ws is null) - return; - if (_ws.State != WebSocketState.Open) + ClientWebSocket? socket = _ws; + if (socket is null || socket.State != WebSocketState.Open) { - try - { - _ = Connect(); - } - catch (Exception e) - { - throw new Exception("Connection is not open."); - } + throw new InvalidOperationException("The WebSocket is not open; a send does not open it."); } - var messagesCount = (int)Math.Ceiling((double)message.Length / SendChunkSize); - - for (var i = 0; i < messagesCount; i++) + await _sendLock.WaitAsync(_cancellationToken).ConfigureAwait(false); + try { - var offset = (SendChunkSize * i); - var count = SendChunkSize; - var lastMessage = ((i + 1) == messagesCount); - - if ((count * (i + 1)) > message.Length) + if (_isIntentionalDisconnect || socket.State != WebSocketState.Open) { - count = message.Length - offset; + throw new InvalidOperationException("The WebSocket was closed before the message could be written."); } - try - { - await _ws.SendAsync(new ArraySegment(message, offset, count), WebSocketMessageType.Binary, lastMessage, _cancellationToken); - } - catch (OperationCanceledException) - { - return; - } - catch (Exception e) + int messagesCount = (int)Math.Ceiling((double)message.Length / SendChunkSize); + + for (int i = 0; i < messagesCount; i++) { - // The send is fire-and-forget (async void), so nothing can observe this exception: - // the pending request just sits there until its RequestTimeout expires. Surface it - // through the error callback - report-only, the connection itself is left alone. - Debug.WriteLine($"{DateTime.Now}WebSocket send failed: {e.GetType().Name}: {e.Message}"); - CallOnError(e); - return; + int offset = SendChunkSize * i; + int count = SendChunkSize; + bool lastMessage = (i + 1) == messagesCount; + + if ((count * (i + 1)) > message.Length) + { + count = message.Length - offset; + } + + await socket.SendAsync(new ArraySegment(message, offset, count), WebSocketMessageType.Binary, lastMessage, _cancellationToken).ConfigureAwait(false); } } + finally + { + _sendLock.Release(); + } } private async Task ConnectAsync() @@ -336,6 +376,17 @@ private async Task ConnectAsync() Dispose(); return; } + catch (Exception) when (_cancellationToken.IsCancellationRequested || _isIntentionalDisconnect) + { + // The handshake was cancelled by this side - the connect-attempt timer, or a + // takeover closing a socket it took - and whoever cancelled it has reported + // already. Not every runtime says so with OperationCanceledException: the browser's + // ClientWebSocket throws a WebSocketException ("ConnectFailure") for a cancelled + // connect, which used to reach the connection-error callback as a second failure + // of the same attempt, with a second OnDisconnect behind it. + _ws?.Dispose(); + _ws = null; + } catch (Exception ex) when (IsNetworkException(ex)) { // Network-related exception during connection handshake - treat as NetworkDrop @@ -623,20 +674,22 @@ private async Task ReceiveLoopAsync() await CallOnDisconnectedAsync(WebSocketCloseStatus.NormalClosure, "Client disconnected").ConfigureAwait(false); return; } - - // Check if this is a network exception (even with nested HttpRequestException) - // before surfacing via error callback - if (IsNetworkException(ex)) + + // A network exception (even with a nested HttpRequestException) is a drop. So is + // any WebSocketException in the browser: its ClientWebSocket surfaces one kind of + // failure for an open socket - the transport going away - and says so with an empty + // message and no HResult, which the pattern check cannot recognise. + if (IsNetworkException(ex) || OperatingSystem.IsBrowser()) { FailureReason = SocketFailureReason.NetworkDrop; await CallOnDisconnectedAsync(WebSocketCloseStatus.EndpointUnavailable, "Network error").ConfigureAwait(false); return; } - - // Not a network exception - surface the error - FailureReason = SocketFailureReason.Unknown; - _onConnectionError?.Invoke(ex, this); - await CallOnDisconnectedAsync(WebSocketCloseStatus.EndpointUnavailable, "WebSocket error: " + ex.Message).ConfigureAwait(false); + + // Not a network exception. Reported as the close it is, and only as that: the + // connection-error callback is for a handshake that failed, and an established + // connection that fails is a closed one - see ReportFailureAsClose. + await ReportFailureAsCloseAsync("WebSocket error: " + Describe(ex)).ConfigureAwait(false); } catch (Exception ex) when (IsNetworkException(ex)) { @@ -656,9 +709,8 @@ private async Task ReceiveLoopAsync() await CallOnDisconnectedAsync(WebSocketCloseStatus.NormalClosure, "Client disconnected").ConfigureAwait(false); return; } - FailureReason = SocketFailureReason.Unknown; - _onConnectionError?.Invoke(ex, this); - await CallOnDisconnectedAsync(WebSocketCloseStatus.EndpointUnavailable, "Unknown error: " + ex.Message).ConfigureAwait(false); + + await ReportFailureAsCloseAsync("Unknown error: " + Describe(ex)).ConfigureAwait(false); } finally { @@ -712,6 +764,39 @@ private void CallOnMessage(byte[] result) } + /// + /// Reports a failure of an established connection - one whose receive loop was running - + /// as the close it is. + /// + /// + /// It used to be reported twice: through the connection-error callback, which is written + /// for a handshake that failed and so announced "Initial connection failed" with a second + /// OnDisconnect behind it, and then through the close callback with the real close + /// code. One event, one report: the close callback classifies the failure from + /// , announces the session end once and starts the reconnect. + /// + private Task ReportFailureAsCloseAsync(string description) + { + FailureReason = SocketFailureReason.Unknown; + return CallOnDisconnectedAsync(WebSocketCloseStatus.EndpointUnavailable, description); + } + + /// + /// The exception's message, or something in its place when the runtime gives none - the + /// browser's carries an empty message and only an error code. + /// + private static string Describe(Exception ex) + { + if (!string.IsNullOrWhiteSpace(ex.Message)) + { + return ex.Message; + } + + return ex is WebSocketException wsEx + ? $"{wsEx.WebSocketErrorCode} ({ex.GetType().Name})" + : ex.GetType().Name; + } + private async Task CallOnDisconnectedAsync(WebSocketCloseStatus? closeStatus = null, string? closeDescription = null) { var handler = _onDisconnected; diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index 5c70fd8a..8e17d03a 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -208,12 +208,21 @@ public class ConnectionOptions /// /// Gets or sets a value indicating whether to enable periodic background health monitoring of the WebSocket connection.
- /// When enabled, the connection state is checked every 20 seconds. If the WebSocket is detected as Closed or Aborted, - /// or if no data has been received for more than 60 seconds, an automatic reconnection is triggered.
+ /// When enabled, the local connection state is checked every . If the WebSocket is + /// detected as no longer Open, an automatic reconnection is triggered.
/// This check does not send any network requests — it only inspects the local connection state.
/// Automatically enabled when is set to .
/// Default: . /// + /// + /// On its own this detects only a socket the runtime already knows is gone. A peer that vanished without + /// closing leaves the socket Open, and the only signal for that is silence - which is a signal only when + /// something is expected to arrive. That is what adds: keepalive pings whose + /// answers keep the activity clock moving, so can mean "the node stopped + /// answering". Without pings an idle connection - no subscriptions, no requests - receives nothing at all, + /// and silence would declare a healthy socket dead every ; so the inactivity + /// check runs only when is enabled. + /// public bool UseCheckHealth { get; set; } = false; /// @@ -232,13 +241,16 @@ public class ConnectionOptions /// /// Gets or sets how long a connection may go without any inbound activity before the health /// check treats it as dead and hands it to the fast-reconnect path.
+ /// Applies only when is enabled.
/// Default: 60 seconds, the threshold this check has always used. /// /// /// A socket whose peer vanished stays Open until the next I/O, so silence is the only - /// signal available without sending traffic. Exposed together with - /// so the fast-reconnect path is reachable from a test in - /// under a second instead of over a minute. + /// signal that reaches the client - and it is a signal only while keepalive pings are being + /// sent, because an idle connection with no subscriptions receives nothing by design. With + /// off this value is not consulted; see the remarks on + /// . Exposed together with so + /// the fast-reconnect path is reachable from a test in under a second instead of over a minute. /// public TimeSpan InactivityTimeout { get; set; } = TimeSpan.FromSeconds(60); @@ -275,28 +287,34 @@ public class ConnectionOptions public int StreamMessageQueueCapacity { get; set; } = 10000; } - private void ValidateConfig() + private void ValidateConfig() => ValidateOptions(config); + + /// + /// Rejects an option set the client cannot run with. Static so can + /// validate the options it was handed before it tears the current connection down. + /// + private static void ValidateOptions(ConnectionOptions options) { - if (config.ConnectionAcquisitionTimeout < config.ConnectionAttemptTimeout) + if (options.ConnectionAcquisitionTimeout < options.ConnectionAttemptTimeout) { throw new ArgumentException( - $"ConnectionAcquisitionTimeout ({config.ConnectionAcquisitionTimeout.TotalSeconds}s) must be >= ConnectionAttemptTimeout ({config.ConnectionAttemptTimeout.TotalSeconds}s) to allow at least one full connection attempt."); + $"ConnectionAcquisitionTimeout ({options.ConnectionAcquisitionTimeout.TotalSeconds}s) must be >= ConnectionAttemptTimeout ({options.ConnectionAttemptTimeout.TotalSeconds}s) to allow at least one full connection attempt."); } // The WASM timer takes this as an int of milliseconds: zero fires once and never repeats, // and anything past int.MaxValue or below zero is rejected outright by the timer itself. // Fail here instead, where the message can say which option is wrong. - double healthCheckMs = config.HealthCheckInterval.TotalMilliseconds; + double healthCheckMs = options.HealthCheckInterval.TotalMilliseconds; if (healthCheckMs < 1 || healthCheckMs > int.MaxValue) { throw new ArgumentException( - $"HealthCheckInterval ({config.HealthCheckInterval}) must be between 1ms and {int.MaxValue}ms."); + $"HealthCheckInterval ({options.HealthCheckInterval}) must be between 1ms and {int.MaxValue}ms."); } - if (config.InactivityTimeout <= TimeSpan.Zero) + if (options.InactivityTimeout <= TimeSpan.Zero) { throw new ArgumentException( - $"InactivityTimeout ({config.InactivityTimeout}) must be positive - a non-positive value would " + + $"InactivityTimeout ({options.InactivityTimeout}) must be positive - a non-positive value would " + "treat every connection as dead on the first health check."); } } @@ -334,8 +352,9 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con public string url { get; private set; } - // Volatile: this is the connectivity gate. It is cleared under _disconnectLock by the - // retirement paths and read lock-free by ShouldBeConnected/State/CheckIfNotConnected. + // Volatile: this is the connectivity gate. It is written only under _transitionLock - by the + // takeover that begins a transition and by ConnectCoreAsync installing that transition's + // socket - and read lock-free by ShouldBeConnected/State/CheckIfNotConnected. public volatile WebSocketClient ws; private int? reconnectTimeoutID = null; @@ -352,41 +371,79 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con private static readonly Random _random = new(); /// - /// Guards the reconnect session — , and - /// — wherever one is read and another written as a unit: - /// , , - /// , and - /// the ownership-guarded writes in . + /// The lock every transition of the connection runs its synchronous part under. The + /// generation counter (), , + /// and the reconnect-loop state + /// (, , + /// ) are written only while it is held. /// /// /// - /// Not every touch of these fields is covered: the per-iteration _reconnectAttempts++ in - /// , the plain resets in ChangeServer and - /// OnceClose, and the "is a loop already running" pre-checks in - /// OnConnectionFailed and OnceClose (which read _reconnectLoop, a - /// non-volatile field, outside the lock) all still run outside it. Those predate this lock; do - /// not read the list above as "all three fields are always synchronized". + /// It absorbs what used to be two locks, _disconnectLock around and + /// _reconnectStateLock around the reconnect session. Kept apart, the two let an + /// operation take the socket under one lock and the reconnect state under the other with a gap + /// in between - the shape of every race in issue #179. stays + /// separate because the per-frame session check takes it, and nests inside this one; so does + /// . The order is fixed: transition, then session, then + /// processor. /// /// - /// volatile alone was not enough: it makes each individual access atomic, not the - /// sequence of them. The stop path used to read the field three times in a row (Cancel, - /// Dispose, null it), so a start running in between could have its brand-new source disposed - /// and cleared by the retiring stop — leaving the loop with a dead source and nobody - /// reconnecting, which is exactly the permanent wedge this whole area exists to prevent. + /// Nothing that can call back into consumer code runs while the lock is held, and nothing + /// awaits under it: a retired cancellation source is cancelled and disposed after the lock is + /// released, the message processor's exit is awaited outside, and the reconnect loop starts + /// with a yield so that starting it under the lock never runs a notification inline. /// + /// + private readonly object _transitionLock = new object(); + + /// + /// The transition that owns the connection right now. Every operation that moves the + /// connection either begins a new one () or continues the current one, + /// and re-validates after every await and every consumer callback with . + /// + /// /// - /// Nothing that can call back into consumer code runs while the lock is held: cancellation and - /// disposal of a retired source happen after the lock is released, and the loop body starts - /// with a yield so that starting it under the lock never runs a notification inline. + /// ChangeServer, Connect, Disconnect, DisconnectAndWaitAsync and + /// the health check's fast reconnect begin a generation: the consumer said something, or the + /// connection was found dead, and whatever was in flight before is over. The socket callbacks + /// (OnceOpen, OnceClose, OnConnectionFailed), the reconnect loop and the + /// failed-OnConnected-handler path continue the generation of the socket they run for: + /// a failed attempt hands the same transition to the loop, a successful one completes it. If + /// that generation is no longer current, a later operation owns the connection, and the + /// callback confines itself to what is unconditionally its own - closing its socket and + /// announcing that its session ended. + /// + /// + /// This replaces the ad-hoc ReferenceEquals(ws, ...) checks that used to reconcile two + /// operations running at once. Those were placed after whichever await somebody had noticed; + /// the generation is checked after all of them, and the socket read and the send happen under + /// the same lock the takeover writes under, so there is no gap for a third operation to fit + /// into. /// /// - private readonly object _reconnectStateLock = new object(); + private long _generation; - // Volatile so the ownership checks in ReconnectLoopAsync can read it outside the lock: - // a single reference read is atomic, and those checks only ever compare, never mutate. - private volatile CancellationTokenSource _reconnectCts; + /// + /// What kind of operation began the current generation. Only used to tell a superseded caller + /// what superseded it. + /// + private TransitionKind _generationKind; + + /// + /// The generation whose reconnect loop is running, or 0 when none is. + /// + /// + /// This is what OnceClose and OnConnectionFailed ask before starting a loop, and + /// what the loop clears - under , together with a re-check that + /// the socket is still open - when its attempt succeeded. It used to be a task reference and an + /// IsCompleted check, which left a window between the loop's break and its task + /// completing where a close saw a running loop that was about to exit and nobody reconnected. + /// + private long _reconnectLoopGeneration; - private Task _reconnectLoop; + // Volatile so the lock-free readers (WaitForConnectionAsync, CheckIfNotConnected) see a + // consistent reference; every write is under _transitionLock. + private volatile CancellationTokenSource _reconnectCts; private SemaphoreSlim _connectLock = new(initialCount: 1, maxCount: 1); @@ -593,13 +650,244 @@ private enum ReconnectMode { None, FastReconnect, LoopReconnect } private TaskCompletionSource? _disconnectTcs = null; - private readonly object _disconnectLock = new(); - // Per-session isolation for ChangeServer private ConnectionSession? _activeSession = null; private readonly object _sessionLock = new(); + /// + /// What began a generation. Carried for the message a superseded caller gets, nothing else. + /// + private enum TransitionKind + { + None, + + Connect, + + ChangeServer, + + Disconnect, + + FastReconnect, + } + + /// + /// What hands the new owner: its generation, the session and socket it + /// took from the previous one, and the exit of the message processor that went with them. + /// + private readonly record struct Takeover( + long Generation, + ConnectionSession? Session, + WebSocketClient? Socket, + Task ProcessorExit); + + /// + /// Whether is still the transition in charge of the connection. + /// + private bool Owns(long generation) => Volatile.Read(ref _generation) == generation; + + private long CurrentGeneration() => Volatile.Read(ref _generation); + + /// + /// Begins a new transition: bumps the generation, takes the live session and socket out of + /// their fields, stops the reconnect loop, the ping timer and the message processor - all in + /// one critical section, so that no other operation can see the connection half taken. + /// + /// + /// The socket leaves here, before the caller sweeps the pending requests: + /// the sweep resumes consumer continuations inline, and a request issued from one of them + /// must already see no usable connection (issue #177). Closing the socket that came out is + /// the caller's job - how it is closed depends on why it was taken. + /// + /// What is taking over. + /// + /// Whether to mark the session retiring, which silences its socket's close callback. + /// Disconnect passes : OnceClose is what announces a user + /// disconnect. + /// + private Takeover TakeOver(TransitionKind kind, bool retireSession) + { + CancellationTokenSource? retiredCts; + Takeover takeover; + lock (_transitionLock) + { + takeover = TakeOverLocked(kind, retireSession, out retiredCts); + } + + retiredCts?.Cancel(); + retiredCts?.Dispose(); + return takeover; + } + + /// + /// for the health check's fast reconnect, which may only take the + /// connection over if the socket it found dead is still the one installed. A user + /// Disconnect() that landed while the ping check was running has taken the socket + /// already, and a reconnect after that would resurrect a client the consumer took down. + /// + /// The socket the ping check found dead. + /// The takeover, when it happened. + /// + /// The cancellation source installed as for this reconnect, so a + /// later takeover can cancel the attempt in flight. + /// + private bool TryTakeOverFrom( + WebSocketClient expectedSocket, + out Takeover takeover, + out CancellationTokenSource ownCts) + { + CancellationTokenSource? retiredCts; + lock (_transitionLock) + { + if (!ReferenceEquals(ws, expectedSocket)) + { + takeover = default; + ownCts = null; + return false; + } + + takeover = TakeOverLocked(TransitionKind.FastReconnect, retireSession: true, out retiredCts); + ownCts = new CancellationTokenSource(); + _reconnectCts = ownCts; + _reconnectAttempts = 1; + _reconnectMode = ReconnectMode.FastReconnect; + _isFastReconnectActive = true; + } + + retiredCts?.Cancel(); + retiredCts?.Dispose(); + return true; + } + + /// + /// The body of . Must be called with held; + /// the retired reconnect source comes out for the caller to cancel and dispose outside it. + /// + private Takeover TakeOverLocked( + TransitionKind kind, + bool retireSession, + out CancellationTokenSource? retiredCts) + { + long generation = ++_generation; + _generationKind = kind; + _permanentlyDisconnected = kind == TransitionKind.Disconnect; + + // The global intentional-disconnect flag follows the generation: set by a disconnect, + // cleared by anything that connects. It used to be cleared only by OnceOpen and by + // ChangeServer, so a Connect() after a Disconnect() ran with it still set, and a failure of + // the new handshake was read as a user disconnect - "closed permanently", no reconnect + // loop, and the caller waiting out ConnectionAcquisitionTimeout for a TimeoutException + // that named nothing. The sockets a disconnect closed stay recognisable through their own + // per-socket marks, which this flag does not touch. + _isIntentionalDisconnect = kind == TransitionKind.Disconnect; + if (kind == TransitionKind.Disconnect) + { + _reconnectMode = ReconnectMode.None; + } + + retiredCts = StopReconnectLoopLocked(); + DetachLocked(retireSession, out ConnectionSession? session, out WebSocketClient? socket); + StopPingTimerSync(); + + (Task? processorTask, CancellationTokenSource? processorCts) detached; + lock (_messageProcessorLock) + { + detached = DetachMessageProcessor(); + } + + return new Takeover( + generation, + session, + socket, + AwaitMessageProcessorExitAsync(detached.processorTask, detached.processorCts)); + } + + /// + /// Takes the session and socket out of their fields. Must be called with + /// held. + /// + /// + /// Nothing is taken when there is no socket: the session then belongs to whoever took the + /// socket before - a Disconnect() whose close callback is about to announce + /// - or to a close that has already been + /// announced. Retiring it here would silence that callback and hand the session to a caller + /// that would announce a reason of its own for an end that was somebody else's. + /// + private void DetachLocked(bool retireSession, out ConnectionSession? session, out WebSocketClient? socket) + { + socket = ws; + ws = null; + + if (socket == null) + { + session = null; + return; + } + + lock (_sessionLock) + { + session = _activeSession; + if (retireSession) + { + session?.MarkAsRetiring(); + } + } + } + + /// + /// Whether will run its close callback once closed - which is only + /// true of a socket whose receive loop exists. A handshake that is cancelled reports nothing, + /// and a socket that is already closed has reported already. + /// + private static bool WillReportClose(WebSocketClient socket) => + socket.State is WebSocketState.Open or WebSocketState.CloseSent or WebSocketState.CloseReceived; + + /// + /// Retires the reconnect loop's state. Must be called with + /// held; the source comes out for the caller to cancel and dispose outside it. A loop that + /// is running notices on its next ownership check and stands down. + /// + private CancellationTokenSource? StopReconnectLoopLocked() + { + CancellationTokenSource? retired = _reconnectCts; + _reconnectCts = null; + _reconnectAttempts = 0; + _reconnectLoopGeneration = 0; + _isFastReconnectActive = false; + return retired; + } + + private void ThrowIfSuperseded(long generation) + { + lock (_transitionLock) + { + ThrowIfSupersededLocked(generation); + } + } + + private void ThrowIfSupersededLocked(long generation) + { + if (_generation != generation) + { + throw SupersededLocked(); + } + } + + /// + /// The exception a superseded caller gets. A Disconnect() that won leaves the client + /// disconnected, and is what every other path says about + /// that; anything else that won is connecting, or connected, somewhere the caller did not ask + /// for, and a cancellation says so without claiming the client is down. + /// + private Exception SupersededLocked() => + _generationKind switch + { + TransitionKind.Disconnect => new NotConnectedException("Client has been disconnected. Call Connect() to reconnect."), + TransitionKind.ChangeServer => new OperationCanceledException($"Superseded by a later ChangeServer to {url}."), + TransitionKind.Connect => new OperationCanceledException("Superseded by a later Connect()."), + _ => new OperationCanceledException("Superseded by a reconnect the health check started."), + }; + public XrpConnectionState CurrentConnectionState => _currentConnectionState; private string _previousNotifiedMessage = string.Empty; @@ -747,77 +1035,89 @@ public Connection(string server, ConnectionOptions? options = null) ValidateConfig(); } + /// + /// Moves the client to another server: retires the current session and connects to + /// . + /// + /// + /// + /// This is a transition of the connection and it can be superseded by a later one - a + /// Disconnect(), another ChangeServer, a Connect() - issued while it is + /// still under way, from another thread or from a consumer callback it runs. It then stops + /// where it is, leaves the rest to the operation that took over, and tells the caller: + /// when a Disconnect() won, because the client is + /// down; when anything else won, because the client + /// is connecting, or connected, somewhere this call did not ask for. It used to reset the + /// disconnect and connect anyway - the client online after the consumer took it down. + /// + /// + /// The old session is retired in the background whatever happens, and the switch is + /// announced through before the new connection is opened. + /// + /// public async Task ChangeServer( string server, ConnectionOptions? options = null, CancellationToken cancellationToken = default) { - SetConnectionState(XrpConnectionState.Connecting, message: $"ChangeServer: Switching to {server}..."); + // Validated before anything is torn down: a bad option set used to be reported only + // after the old connection was gone. + ValidateOptions(options ?? config); - // ===================================================== - // FAST CHANGE SERVER with PER-SESSION ISOLATION - // ===================================================== - // Old session is marked as retiring and cleaned up in background. - // New session is created immediately without waiting. - // Callbacks check session ID to ignore retiring sessions. - - // 1. Quick state cleanup - stop reconnect loop - StopReconnectLoop(); - - // 2. Cancel ping timer and the message processor (but don't wait yet). Stopping the - // processor is explicit since StopPingTimerSync no longer does it as a side effect - this - // session's queue goes with the session. - StopPingTimerSync(); + Takeover takeover = TakeOver(TransitionKind.ChangeServer, retireSession: true); + long generation = takeover.Generation; - // 3. Mark old session as retiring (callbacks will be ignored) and clear ws - BEFORE - // rejecting the pending requests, on purpose. - // The rejection sweep resumes consumer continuations - inline on this thread when there - // is no synchronization context - and a consumer that issues its next request from there - // must already see no usable connection. Cleared afterwards, that request passes the - // connectivity check on the old socket and is written into it after the sweep that would - // have rejected it; nothing completes it before RequestTimeout (issue #177). - ConnectionSession? oldSession; - lock (_sessionLock) + // The old socket is closed in the background whatever happens below. A call superseded at + // one of the checks still owes the server it left a close frame - nobody else holds the + // socket any more. + // + // Per-socket tracking only. The global _isIntentionalDisconnect flag was only reset in + // OnceOpen, so if the NEW server never came up it stayed set forever: OnConnectionFailed + // then read the failure of the new socket as a user disconnect and started no reconnect + // loop, leaving the client dead with "No connection attempt in progress." + if (takeover.Socket != null) { - oldSession = _activeSession; - oldSession?.MarkAsRetiring(); + Interlocked.Exchange(ref _userInitiatedSocket, takeover.Socket); + MarkSocketAsUserInitiated(takeover.Socket); + _ = RetireOldSessionAsync(takeover.Session, takeover.Socket); } - - WebSocketClient? oldSocket; - lock (_disconnectLock) + else { - oldSocket = ws; - ws = null; + takeover.Session?.CompleteSession(); } - // 4. Reject all pending requests BEFORE waiting for ping - // This allows the ping handler to receive OperationCanceledException and exit quickly - requestManager.RejectAllWithCancellation(); - connectionManager.RejectAllAwaitingWithCancellation(); + string sessionEnded = $"Switched to {server}. Subscriptions from the previous connection are no longer in effect."; - // 5. The message processor goes with the session, and its reader is let go of after the - // sweep, not before: consumers are released first, and this is the first yield of the - // switch - what a single-threaded host runs their continuations on. - await StopMessageProcessorAsync(); + try + { + // Notified after the takeover, not before it: a handler that answers this with + // Disconnect() has to win, and it can only win against a transition that has begun. + SetConnectionState(XrpConnectionState.Connecting, message: $"ChangeServer: Switching to {server}..."); + ThrowIfSuperseded(generation); - // 6. Now wait for ping to finish (should be very fast since requests were rejected) - await WaitForPingToFinishAsync(); + // The takeover cleared ws before this sweep, on purpose: the sweep resumes consumer + // continuations - inline on this thread when there is no synchronization context - and + // a request issued from one of them must already see no usable connection (issue #177). + requestManager.RejectAllWithCancellation(); + connectionManager.RejectAllAwaitingWithCancellation(); + ThrowIfSuperseded(generation); - // 7. Mark old socket for intentional disconnect (per-socket tracking only) - // CRITICAL: Do NOT set global _isIntentionalDisconnect = true here - same rule as the ping/network - // recovery path. The global flag was only reset in OnceOpen, so if the NEW server never came up it - // stayed set forever: OnConnectionFailed then read the failure of the new socket as a user disconnect, - // reported "Connection closed permanently." and started no reconnect loop, leaving the client dead - // with the misleading "No connection attempt in progress. Call Connect() first." - // Per-socket tracking (_userInitiatedSockets + the socket's own flag, set in RetireOldSessionAsync) - // already filters late callbacks from the old socket, and keeps global state clean for the new one. - if (oldSocket != null) - { - Interlocked.Exchange(ref _userInitiatedSocket, oldSocket); - MarkSocketAsUserInitiated(oldSocket); + // The message processor went with the session, and its reader is let go of after the + // sweep, not before: consumers are released first, and this is the first yield of the + // switch - what a single-threaded host runs their continuations on. + await takeover.ProcessorExit; + ThrowIfSuperseded(generation); - // 6. Fire-and-forget GRACEFUL disposal - no blocking - _ = RetireOldSessionAsync(oldSession, oldSocket); + await WaitForPingToFinishAsync(); + ThrowIfSuperseded(generation); + } + catch + { + // Superseded before the switch was announced. The session was retired by the takeover, + // which silences its own close callback, and the operation that took over does not + // know it - so the announcement the consumer is owed goes out here, or never. + await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ServerChanged, sessionEnded); + throw; } // The consumer's subscriptions belonged to the session just retired and do not follow the @@ -826,32 +1126,33 @@ public async Task ChangeServer( // Connecting, which is what a first connection reports too. Announced before the new // connection is opened, so a consumer cannot see OnConnected for the new session and only // afterwards learn that the old one is gone. - await NotifySessionEndedAsync( - oldSession, - SessionEndReason.ServerChanged, - $"Switched to {server}. Subscriptions from the previous connection are no longer in effect."); + await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ServerChanged, sessionEnded); - // 7. Update config for new server - url = server; - if (options != null) + // The handler above may have started something of its own; the target is written only by + // the transition that still owns the connection. + lock (_transitionLock) { - config = options; + ThrowIfSupersededLocked(generation); + url = server; + if (options != null) + { + config = options; + } } - ValidateConfig(); - _reconnectAttempts = 0; Interlocked.Exchange(ref _connectHandlerFailures, value: 0); - // 8. Reset permanentlyDisconnected for new connection - _permanentlyDisconnected = false; - - // Clear the global intentional-disconnect flag explicitly: it may still be set from an earlier - // user Disconnect() (it is only ever reset in OnceOpen), and leaving it set would make a failure - // of the NEW connection look intentional and suppress reconnection. - _isIntentionalDisconnect = false; + await ConnectCoreAsync(generation, cancellationToken); + await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, cancellationToken); - // 9. Immediately connect to new server (new session created in Connect) - await Connect(cancellationToken); + // Connected - but a later ChangeServer that took over during the wait connected to its own + // server, and this call's is not where the client is. + string connectedTo = GetUrl(); + if (!string.Equals(connectedTo, server, StringComparison.Ordinal)) + { + throw new OperationCanceledException( + $"ChangeServer to {server} was superseded by a later ChangeServer to {connectedTo}."); + } } /// @@ -875,219 +1176,155 @@ private async Task RetireOldSessionAsync(ConnectionSession? session, WebSocketCl } /// - /// Retires current session and reconnects immediately (same flow as ChangeServer). + /// Retires the current session and reconnects immediately (same flow as ChangeServer). /// Used for ping timeout and network drop to avoid slow reconnect with exponential backoff. /// - private async Task RetireCurrentSessionAndReconnectAsync(string reason) + /// + /// + /// Runs inside the ping check that found dead. It takes the + /// connection over only if that socket is still the one installed: a user + /// Disconnect() that landed during the check has taken it already, and reconnecting + /// after that would resurrect a client the consumer took down. It used to check + /// _permanentlyDisconnected at two points along the way instead, and the review of + /// #178 found the point in between. + /// + /// + /// The session and socket are captured by the takeover, before the + /// notification - not after it. A + /// handler that answers that notification with a ChangeServer and blocks on it has + /// installed a replacement session by the time the callback returns; captured then, the + /// replacement would be the one marked retiring. Now the handler's switch takes the + /// connection over, and this path stands down at the check that follows the callback. + /// + /// + private async Task RetireCurrentSessionAndReconnectAsync(string reason, WebSocketClient deadSocket) { - // ===================================================== - // CRITICAL: Set reconnect state FIRST so IsReconnectActive() returns true - // throughout the entire operation, including if Connect() fails. - // ===================================================== - - // 1. Set reconnect mode FIRST - this is the authoritative state - // It will be cleared only when connection is stable (in OnceOpen) - _reconnectMode = ReconnectMode.FastReconnect; - _isFastReconnectActive = true; // Keep for backward compatibility - - // 2-3. Retire the previous reconnect session and install this one as a single transaction, - // so a concurrent stop/start cannot dispose the source created here. Cancellation and - // disposal of the old source happen after the lock is released. - CancellationTokenSource oldCts; - CancellationTokenSource ownCts; - lock (_reconnectStateLock) - { - oldCts = _reconnectCts; - _reconnectLoop = null; // Clear old loop reference so StartReconnectLoop can start a new one - _reconnectAttempts = 1; - ownCts = new CancellationTokenSource(); - _reconnectCts = ownCts; + if (!TryTakeOverFrom(deadSocket, out Takeover takeover, out CancellationTokenSource ownCts)) + { + Debug.WriteLine($"{DateTime.Now}Fast reconnect not started - the socket found dead is no longer the connection"); + return; + } + + long generation = takeover.Generation; + + // Per-socket tracking only - see ChangeServer for why the global flag stays clear. Closed + // in the background before anything else, so a stand-down below leaves nothing open. + if (takeover.Socket != null) + { + Interlocked.Exchange(ref _userInitiatedSocket, takeover.Socket); + MarkSocketAsUserInitiated(takeover.Socket); + takeover.Socket.SetIntentionalDisconnect(); // Suppresses Critical logging in receive loop + _ = RetireOldSessionAsync(takeover.Session, takeover.Socket); + } + else + { + takeover.Session?.CompleteSession(); } - oldCts?.Cancel(); - oldCts?.Dispose(); - - // 4. Now send first notification - IsReconnectActive() will return true // Consumer handler exceptions are contained inside SetConnectionState - an escaping throw - // here would leave the source installed above with no loop and nobody to dispose it. + // here would leave the source installed by the takeover with no loop and nobody to + // dispose it. SetConnectionState( XrpConnectionState.RestoringConnection, message: $"{reason} Reconnecting immediately...", ConnectionCloseSeverity.Warning, reconnect: BuildReconnectInfo()); - // ===================================================== - // FAST RECONNECT with PER-SESSION ISOLATION (same as ChangeServer) - // ===================================================== - // Old session is marked as retiring and cleaned up in background. - // New session is created immediately without waiting. - // Callbacks check session ID to ignore retiring sessions. - - // 4. Stop ping timer and the message processor (but don't wait yet) - the queue belongs - // to the session being retired. - StopPingTimerSync(); - - // 5. Mark old session as retiring (callbacks will be ignored) and clear ws - BEFORE - // rejecting the pending requests, on purpose. - // The rejection sweep resumes consumer continuations - inline on this thread when there - // is no synchronization context - and a consumer that issues its next request from there - // must already see no usable connection. Cleared afterwards, that request passes the - // connectivity check on the old socket and is written into it after the sweep that would - // have rejected it; nothing completes it before RequestTimeout (issue #177). - ConnectionSession? oldSession; - lock (_sessionLock) - { - oldSession = _activeSession; - oldSession?.MarkAsRetiring(); - } - - WebSocketClient? oldSocket; - lock (_disconnectLock) + // Standing down before the session end is announced still owes that announcement: the + // takeover retired the session, which silences its own close callback, and whoever took + // over does not know the session - see the same catch in ChangeServer. + if (!Owns(generation)) { - oldSocket = ws; - ws = null; + Debug.WriteLine($"{DateTime.Now}Fast reconnect superseded from the status handler"); + await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ConnectionLost, reason).ConfigureAwait(false); + return; } - // 6. Reject all pending requests BEFORE waiting for ping - // This allows the ping handler to receive OperationCanceledException and exit quickly + // ws is already null, so a request issued from a rejected continuation sees no usable + // connection (issue #177). The rejection also lets the ping handler exit quickly. requestManager.RejectAllWithCancellation(); connectionManager.RejectAllAwaitingWithCancellation(); - // 7. The message processor goes with the session (see ChangeServer for the ordering). - await StopMessageProcessorAsync().ConfigureAwait(false); - - // 8. Now wait for the ping to finish. This method runs inside the ping check itself, so - // the wait returns at once - see WaitForPingToFinishAsync. - await WaitForPingToFinishAsync().ConfigureAwait(false); - - // 9. Mark old socket for intentional disconnect (per-socket tracking only) - // CRITICAL: Do NOT set global _isIntentionalDisconnect = true for ping/network recoveries! - // The global flag would block OnConnectionFailed from processing new connection failures. - // Instead, rely solely on per-socket tracking (_userInitiatedSockets HashSet) to filter - // late callbacks from the old socket while keeping global state clean for the new connection. - if (oldSocket != null) + if (!Owns(generation)) { - // Per-socket tracking - filters late callbacks from this specific socket - Interlocked.Exchange(ref _userInitiatedSocket, oldSocket); - MarkSocketAsUserInitiated(oldSocket); - oldSocket.SetIntentionalDisconnect(); // Suppresses Critical logging in receive loop - - // 9. Fire-and-forget GRACEFUL disposal - no blocking - _ = RetireOldSessionAsync(oldSession, oldSocket); + await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ConnectionLost, reason).ConfigureAwait(false); + return; } + // The message processor went with the session (see ChangeServer for the ordering). + await takeover.ProcessorExit.ConfigureAwait(false); + + // This method runs inside the ping check itself, so the wait returns at once - see + // WaitForPingToFinishAsync. + await WaitForPingToFinishAsync().ConfigureAwait(false); + // Same as ChangeServer: the session being retired took the subscriptions with it, and the // socket's own close callback will be filtered out as retiring. The RestoringConnection // status above reports that the connection is being rebuilt, not that everything bound to - // the old one is gone - a consumer had to infer the second from the first. - await NotifySessionEndedAsync(oldSession, SessionEndReason.ConnectionLost, reason) + // the old one is gone - a consumer had to infer the second from the first. Announced + // whether or not this path still owns the connection, for the reason given above. + await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ConnectionLost, reason) .ConfigureAwait(false); - // 10. Clear ping/network drop socket tracking (old socket is retired) - // CRITICAL: If not cleared, these stale references would cause OnConnectionFailed - // to filter callbacks from the NEW socket if Connect() fails, blocking reconnection. - _pingTimeoutSocket = null; - _networkDropSocket = null; - - // 11. Reset permanentlyDisconnected for new connection - unless the user asked to disconnect - // while the awaits above were running. Disconnect() sets the flag, clears the reconnect - // state and then waits on the ping task this method runs inside, so it is still blocked - // here and cannot have finished its teardown. Clearing its flag and reconnecting anyway - // would resurrect a client the consumer explicitly took down - and Disconnect() would - // return reporting success while a fresh session was being built behind it. - if (_permanentlyDisconnected) + if (!Owns(generation)) { - CancellationTokenSource abandoned = null; - lock (_reconnectStateLock) - { - if (ReferenceEquals(_reconnectCts, ownCts)) - { - abandoned = ownCts; - _reconnectCts = null; - _reconnectAttempts = 0; - _reconnectLoop = null; - } - } - - abandoned?.Cancel(); - abandoned?.Dispose(); - _isFastReconnectActive = false; - - Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned before connecting - the client was disconnected by the user"); return; } - _permanentlyDisconnected = false; - - // Note: _reconnectAttempts and _reconnectCts already set at the start of this method - // Global _isIntentionalDisconnect stays false - allows new connection failures to be processed + // Clear ping/network drop socket tracking (old socket is retired). If not cleared, these + // stale references would cause OnConnectionFailed to filter callbacks from the NEW socket + // if the attempt fails, blocking reconnection. + _pingTimeoutSocket = null; + _networkDropSocket = null; - // 12. Immediately connect (bypass Connect() which calls StopReconnectLoop) - // Note: Don't emit Connecting state here - we already emitted RestoringConnection - // and Connecting would overwrite ReconnectInfo, confusing consuming apps + // Don't emit Connecting state here - RestoringConnection has been emitted already, and + // Connecting would overwrite ReconnectInfo, confusing consuming apps. try { - // Pass the token of the session this method owns: a user Disconnect() cancels it, so the - // attempt below stops instead of opening a socket behind a client that was taken down. - // Disconnect() waits only briefly for the ping task, while acquisition can run much - // longer, so the flag check above cannot cover this window on its own. - await ConnectInternalAsync(ownCts.Token).ConfigureAwait(false); + // The token of the source the takeover installed: a later takeover - a user + // Disconnect(), say - cancels it, so the attempt below stops instead of opening a + // socket behind a client that was taken down. + await ConnectCoreAsync(generation, ownCts.Token).ConfigureAwait(false); await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, ownCts.Token).ConfigureAwait(false); - - // Connect succeeded - cleanup reconnect state - // Note: _reconnectMode will be cleared in OnceOpen when connection is fully established + _isFastReconnectActive = false; - // Only tear down the source this method installed. The awaits above give a concurrent - // path (RestartReconnectLoop from a failing OnConnected handler, say) room to install a - // newer one; cancelling and disposing that would strand the sequence it belongs to, - // which is the same wedge the ownership checks in ReconnectLoopAsync guard against. - // When ownership is lost, ownCts needs no cleanup here: whoever evicted it from the - // field cancelled and disposed it as part of doing so. + // Only tear down the source this path installed, and only while it still owns the + // connection and no loop is running on the source: the attempt above may have failed + // at the socket, the failure callback started the loop on this same source, and the + // loop is the one that connected - it releases the source itself. CancellationTokenSource settled = null; - lock (_reconnectStateLock) + lock (_transitionLock) { - if (ReferenceEquals(_reconnectCts, ownCts)) + if (Owns(generation) && + _reconnectLoopGeneration != generation && + ReferenceEquals(_reconnectCts, ownCts)) { settled = ownCts; _reconnectCts = null; _reconnectAttempts = 0; - - // Drop the task reference in the same transaction, for the same reason - // StopReconnectLoop does: a loop may have been started on this very source - // while the awaits above were running (OnConnectionFailed sees no live loop - - // the entry above cleared the reference - and StartReconnectLoop reuses a - // still-valid source). Cancelling that source without clearing the reference - // leaves every loopIsRunning check looking at a task that is exiting, so - // nobody starts a replacement and nobody reconnects. - _reconnectLoop = null; } } - settled?.Cancel(); settled?.Dispose(); } catch (Exception ex) { - // If Connect fails, transition to loop reconnect mode - // Keep _reconnectMode set (will be LoopReconnect after StartReconnectLoop) - - // A user Disconnect() can land while the awaits above are running - and it will wait on - // the very ping task this method runs inside, so it cannot have finished yet. Handing - // the client back to a reconnect loop then would undo an explicit disconnect. The flag - // is the authority: leave the state alone and let Disconnect() finish its teardown. - if (_permanentlyDisconnected) + // A later transition owns the connection - a user Disconnect(), a ChangeServer from a + // handler. Handing the client to a reconnect loop now would undo what it did. + if (!Owns(generation)) { - Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned - the client was disconnected by the user: {ex.Message}"); + Debug.WriteLine($"{DateTime.Now}Fast reconnect superseded: {ex.Message}"); return; } - // The wait above ends in cancellation when its own source is retired, and on success the - // path that retires it is OnceOpen: the attempt above failed at the socket, the failure - // callback started the reconnect loop on this same source, and that loop connected - // first. A client that is connected has nothing to reconnect. Treating this as a failure - // started a second loop, whose first attempt retired the live socket and opened another - - // one reconnect became two, with a RestoringConnection reported on a healthy client. + // The wait above ends in cancellation when its source is released, and on success the + // path that releases it is the reconnect loop: the attempt above failed at the socket, + // the failure callback started the loop on this same source, and that loop connected + // first. A client that is connected has nothing to reconnect. Treating this as a + // failure started a second loop, whose first attempt retired the live socket and + // opened another - one reconnect became two, with a RestoringConnection reported on a + // healthy client. if (IsConnected()) { _isFastReconnectActive = false; @@ -1095,17 +1332,21 @@ await NotifySessionEndedAsync(oldSession, SessionEndReason.ConnectionLost, reaso return; } + // The wait says the client gave up: the loop the failure callback started on this + // source ran out of attempts, reported Disconnected and released the source. Starting + // another loop here would run a second full series behind a state that said the first + // was the last, and StopAfterMaxAttempts would mean nothing. + if (ex is NotConnectedException) + { + _isFastReconnectActive = false; + Debug.WriteLine($"{DateTime.Now}Fast reconnect gave up with the reconnect loop: {ex.Message}"); + return; + } + // Start the loop BEFORE notifying: SetConnectionState calls into consumer code, and an // exception from a handler must not cost us the reconnect loop. Ordering matters more // than the message here - without the loop the client never comes back. - // - // StartReconnectLoop reuses the source installed above when it is still there. It may - // not be: the awaits could have let another path replace or clear it, the same way the - // success branch above can no longer assume it still owns ownCts. Either outcome is - // survivable here - a live foreign loop makes the call return early, a cleared source - // makes it start a fresh sequence (losing only the seeded first delay) - so this path - // does not need an ownership check of its own. - StartReconnectLoop(); + StartReconnectLoop(generation); SetConnectionState( XrpConnectionState.RestoringConnection, @@ -1209,74 +1450,120 @@ public async Task Connect(CancellationToken cancellationToken) return; } - StopReconnectLoop(); + // Connect() is the consumer saying "connect now", and that wins over whatever the client + // was doing on its own: the reconnect loop stops, and a handshake another transition still + // has in flight is closed - the takeover took it out of ws, so it is this call's to close. + Takeover takeover = TakeOver(TransitionKind.Connect, retireSession: true); + if (takeover.Socket != null) + { + MarkSocketAsUserInitiated(takeover.Socket); + CloseSocketIntentionally(takeover.Socket); + } + + takeover.Session?.CompleteSession(); + + // Whatever was written to that socket is not going to be answered. Its close callback + // would have swept these, but a close that is still being processed when this takeover + // lands finds the connection owned by someone else and leaves the sweep to that owner - + // and that owner is this call. + requestManager.RejectAllWithCancellation(); + + // The previous session's reader may still be inside a consumer handler; the new + // connection's reader must not run alongside it. Completed at once on a client that had + // no processor, bounded on one that did - see AwaitMessageProcessorExitAsync. + await takeover.ProcessorExit; + Interlocked.Exchange(ref _connectHandlerFailures, value: 0); SetConnectionState(XrpConnectionState.Connecting, message: $"Connecting to {url}..."); - await ConnectInternalAsync(); + + // A session that had opened held the consumer's subscriptions, and retiring it above + // silences the close callback that would otherwise have announced their loss - the same + // reason ChangeServer announces for itself. A session that never opened announces + // nothing. + await NotifySessionEndedAsync( + takeover.Session, + SessionEndReason.ConnectionLost, + "Connect() replaced a connection that was no longer open. Subscriptions from the previous connection are no longer in effect."); + + try + { + await ConnectCoreAsync(takeover.Generation); + } + catch (OperationCanceledException) + { + // Superseded by a later ChangeServer or Connect(): that operation is connecting now, + // and the wait below reports on its outcome. OperationCanceledException from this + // method means the caller's own token and nothing else - see IXrplClient.Connect. A + // Disconnect() that won comes out of ConnectCoreAsync as NotConnectedException and + // propagates. + } + await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, cancellationToken); } - private async Task ConnectInternalAsync(CancellationToken ct = default) + /// + /// Opens the socket for . The caller has taken the connection + /// over (or continues a transition that did) and is null. + /// + /// + /// + /// Ownership is checked under at three points: after the connect + /// lock is acquired, together with the installation of the socket, and after the handshake. + /// The middle one is what makes a socket created during a race impossible: the takeover and + /// the installation run under the same lock, so a takeover either finds + /// empty - and this method, seeing the generation move on, never creates the socket - or + /// finds the socket installed and takes it. The last one covers a takeover that landed while + /// the handshake ran: the socket is this method's to close, and it closes it. + /// + /// + /// A takeover before the socket exists is reported as an exception + /// (); one after the handshake is not - the socket is closed and + /// the method returns, leaving the caller's wait to report on whatever the new owner does. + /// + /// + /// The transition this attempt belongs to. + /// + /// Cancels the attempt: the reconnect loop's and the fast reconnect's source, which a later + /// takeover cancels. + /// + private async Task ConnectCoreAsync(long generation, CancellationToken ct = default) { - _permanentlyDisconnected = false; await _connectLock.WaitAsync(ct); try { - // Check cancellation before proceeding ct.ThrowIfCancellationRequested(); - - if (IsConnected()) - { - return; - } - if (State() == WebSocketState.Connecting) + WebSocketClient capturedSocket; + ConnectionSession capturedSession; + lock (_transitionLock) { - await connectionManager.AwaitConnection(); - return; - } + ThrowIfSupersededLocked(generation); - if (url == null) - { - throw new ConnectionException("Cannot connect because no server was specified"); - } - - if (this.ws != null) - { - throw new XrplException("Websocket connection never cleaned up."); - } - - // Check cancellation again before creating WebSocket - ct.ThrowIfCancellationRequested(); - - this.ws = CreateWebSocket(url, config); - _lastActiveSocket = this.ws; - var capturedSocket = this.ws; + if (ShouldBeConnected()) + { + return; + } - // Check cancellation AFTER creating WebSocket - if cancelled, close the socket and exit - if (ct.IsCancellationRequested) - { - try + if (url == null) { - capturedSocket?.SetIntentionalDisconnect(); - _ = capturedSocket?.InitiateGracefulCloseAsync(); + throw new ConnectionException("Cannot connect because no server was specified"); } - catch { /* swallow */ } - finally + + if (ws != null) { - this.ws = null; + throw new XrplException("Websocket connection never cleaned up."); } - ct.ThrowIfCancellationRequested(); - } - // Create session for this connection - var newSession = new ConnectionSession(this.ws); - lock (_sessionLock) - { - _activeSession = newSession; - } + capturedSocket = CreateWebSocket(url, config); + ws = capturedSocket; + _lastActiveSocket = capturedSocket; - var capturedSession = newSession; + capturedSession = new ConnectionSession(capturedSocket, generation); + lock (_sessionLock) + { + _activeSession = capturedSession; + } + } timer = new Timer(config.ConnectionAttemptTimeout.TotalMilliseconds); timer.Elapsed += async (sender, e) => @@ -1295,12 +1582,9 @@ await OnConnectionFailed( } }; timer.Start(); - if (this.ws == null) - { - throw new XrplException("Connect: created null websocket"); - } + Timer capturedTimer = timer; - ws.OnConnect(async (connectedSocket) => + capturedSocket.OnConnect(async (connectedSocket) => { try { @@ -1312,8 +1596,7 @@ await OnConnectionFailed( } }); - var capturedTimer = timer; - ws.OnConnectionError(async (e, errorSocket) => + capturedSocket.OnConnectionError(async (e, errorSocket) => { try { @@ -1331,7 +1614,7 @@ await OnConnectionFailed( } }); - ws.OnError(async (e, errorSocket) => + capturedSocket.OnError(async (e, errorSocket) => { try { @@ -1357,7 +1640,7 @@ await errorHandler.Invoke( // Bound to the binary callback rather than the string one: the frame is already UTF-8 // and that is what the JSON reader wants, so the UTF-16 copy of every message - twice // the byte length, on the large object heap for a big response - is never made. - ws.OnBinaryMessage(async (m, ws) => + capturedSocket.OnBinaryMessage(async (m, _) => { try { @@ -1373,7 +1656,7 @@ await errorHandler.Invoke( Debug.WriteLine($"{DateTime.Now}OnBinaryMessage callback error: {ex.Message}"); } }); - ws.OnDisconnect(async (closeStatus, closeDescription, closingSocket) => + capturedSocket.OnDisconnect(async (closeStatus, closeDescription, closingSocket) => { try { @@ -1392,9 +1675,38 @@ await errorHandler.Invoke( } }); - await this.ws.Connect(); + await capturedSocket.Connect(); + + // The handshake is over, one way or another, and the attempt timer has nothing left + // to time. On success and on failure the socket's callbacks stopped it already; a + // handshake that a takeover cancelled reports nothing at all - WebSocketClient + // swallows the cancellation - and the timer would go on firing OnConnectionFailed + // for this dead socket at every ConnectionAttemptTimeout, completing whatever + // disconnect source a later DisconnectAndWaitAsync had installed. + capturedTimer.Stop(); + capturedTimer.Dispose(); + + // A takeover during the handshake took ws - or found it empty, if the handshake had + // already failed and its callback cleared it. A socket that is open is nobody's now + // but this method's, and it must not be left open behind the new owner. One that is + // not open has been dealt with by its callback, or by the takeover that cancelled it; + // marking it again here would re-add it to the user-initiated set after that callback + // removed it, with no close left to take it out. + bool superseded; + lock (_transitionLock) + { + superseded = !Owns(generation); + if (superseded && ReferenceEquals(ws, capturedSocket)) + { + ws = null; + } + } - connectionManager.AwaitConnection(); + if (superseded && capturedSocket.State == WebSocketState.Open) + { + MarkSocketAsUserInitiated(capturedSocket); + CloseSocketIntentionally(capturedSocket); + } } finally { @@ -1402,52 +1714,104 @@ await errorHandler.Invoke( } } - public async Task Disconnect() + /// + /// Takes the connection over on behalf of a user disconnect and marks the socket that came + /// out - the part and share. + /// + /// + /// A disconnect wins against anything in flight: the takeover bumps the generation, so a + /// ChangeServer or a reconnect that was mid-way stands down at its next check and a + /// handshake it had running is closed by the attempt that started it. Nothing resets the + /// disconnect afterwards except the consumer's own Connect() or ChangeServer. + /// + /// + /// The takeover and the completion source the socket's close callback completes, when there + /// was a socket to close. + /// + private (Takeover Takeover, TaskCompletionSource? Tcs) TakeOverForDisconnect() { - _isIntentionalDisconnect = true; - _permanentlyDisconnected = true; - - // Capture the socket and clear ws BEFORE rejecting the pending requests - see ChangeServer - // for why: the sweep runs consumer continuations, and a request issued from one of them - // must not find the socket being closed still installed as the connection (issue #177). - WebSocketClient? socketToClose; - lock (_disconnectLock) + // The socket is marked and the completion source installed in the same critical section + // that takes the socket: its close callback completes the source, and a peer closing the + // socket in the instant between would otherwise find no source to complete and leave + // DisconnectAndWaitAsync waiting out its timeout. + Takeover takeover; + TaskCompletionSource? tcs = null; + CancellationTokenSource? retiredCts; + lock (_transitionLock) { - socketToClose = ws; - ws = null; + takeover = TakeOverLocked(TransitionKind.Disconnect, retireSession: false, out retiredCts); + WebSocketClient? socketToClose = takeover.Socket; if (socketToClose != null) { MarkSocketAsUserInitiated(socketToClose); socketToClose.SetIntentionalDisconnect(); - if (_disconnectTcs == null || _disconnectTcs.Task.IsCompleted) + // Only for a socket whose close will be reported. A source installed for a + // handshake in flight is completed by nobody - the cancelled handshake reports + // nothing - and the next DisconnectAndWaitAsync would wait out its timeout on it. + if (WillReportClose(socketToClose)) { - _disconnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + if (_disconnectTcs == null || _disconnectTcs.Task.IsCompleted) + { + _disconnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + } + + tcs = _disconnectTcs; } } } - ClearReconnectState(); // Clear all reconnect state on user disconnect - StopPingTimerSync(); + retiredCts?.Cancel(); + retiredCts?.Dispose(); + + return (takeover, tcs); + } + + public async Task Disconnect() + { + (Takeover takeover, _) = TakeOverForDisconnect(); + long generation = takeover.Generation; + WebSocketClient? socketToClose = takeover.Socket; - // Reject pending requests so ping handler can exit quickly + // ws left the field in the takeover, before this sweep, so a request issued from a + // rejected continuation finds no socket to go into (issue #177). The rejection also lets + // the ping handler exit quickly. requestManager.RejectAllWithCancellation(); connectionManager.RejectAllAwaitingWithCancellation(); - await StopMessageProcessorAsync(); + await takeover.ProcessorExit; await WaitForPingToFinishAsync(); if (socketToClose == null) { - SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + // Reported only while this disconnect still owns the connection: a Connect() or + // ChangeServer that took over during the awaits above is reporting its own state now. + if (Owns(generation)) + { + SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + } + return 0; } Interlocked.Exchange(ref _userInitiatedSocket, socketToClose); CloseSocketIntentionally(socketToClose); - SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + if (Owns(generation)) + { + SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + } + + // Announced here as well as from the socket's close callback, which dedups. The callback + // alone is not enough: a Connect() issued right after this call installs a new session + // before the old socket's close is processed, and the callback then files it as a stale + // session and announces nothing - the consumer bounced the client and never heard that + // its subscriptions went with the old connection. + await NotifySessionEndedAsync( + takeover.Session, + SessionEndReason.UserDisconnected, + "Disconnected by user request. Subscriptions from this connection are no longer in effect."); return 0; } @@ -1459,49 +1823,25 @@ public async Task Disconnect() /// Cancellation token. public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken cancellationToken = default) { - _isIntentionalDisconnect = true; - _permanentlyDisconnected = true; - - // Same ordering as Disconnect(): the socket leaves ws before the sweep runs (issue #177). - TaskCompletionSource? tcs = null; - WebSocketClient? socketToClose; - lock (_disconnectLock) - { - socketToClose = ws; - ws = null; - - if (socketToClose != null) - { - MarkSocketAsUserInitiated(socketToClose); - socketToClose.SetIntentionalDisconnect(); - - if (_disconnectTcs == null || _disconnectTcs.Task.IsCompleted) - { - _disconnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - } - - tcs = _disconnectTcs; - } - } - - ClearReconnectState(); // Clear all reconnect state on user disconnect - StopPingTimerSync(); + (Takeover takeover, TaskCompletionSource? tcs) = TakeOverForDisconnect(); + long generation = takeover.Generation; + WebSocketClient? socketToClose = takeover.Socket; - // Reject pending requests so ping handler can exit quickly + // Same ordering as Disconnect(): the socket left ws before the sweep runs (issue #177). requestManager.RejectAllWithCancellation(); connectionManager.RejectAllAwaitingWithCancellation(); - await StopMessageProcessorAsync(); + await takeover.ProcessorExit; await WaitForPingToFinishAsync(); - if (socketToClose == null || tcs == null) + if (socketToClose == null) { // Nothing here to close - but another DisconnectAndWaitAsync may be mid-way, having // taken the socket already. This call promised to return once the socket is gone, so // it waits on that one's completion source rather than reporting a disconnect that // has not finished. TaskCompletionSource? inProgress; - lock (_disconnectLock) + lock (_transitionLock) { inProgress = _disconnectTcs; } @@ -1511,13 +1851,34 @@ public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken can await Task.WhenAny(inProgress.Task, Task.Delay(timeout, cancellationToken)); } - SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + if (Owns(generation)) + { + SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + } + return; } Interlocked.Exchange(ref _userInitiatedSocket, socketToClose); - SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + if (Owns(generation)) + { + SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + } + + // See Disconnect() for why this is announced here and not left to the close callback. + await NotifySessionEndedAsync( + takeover.Session, + SessionEndReason.UserDisconnected, + "Disconnected by user request. Subscriptions from this connection are no longer in effect."); + + if (tcs == null) + { + // A handshake in flight: closing it reports nothing, so there is nothing to wait for + // once the cancellation is issued. + CloseSocketIntentionally(socketToClose); + return; + } // Start disconnect async - it waits for receive loop which calls OnceClose // OnceClose will complete tcs, so both should complete around the same time @@ -1558,7 +1919,7 @@ public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken can private void CompleteDisconnectTcs() { - lock (_disconnectLock) + lock (_transitionLock) { _disconnectTcs?.TrySetResult(true); _disconnectTcs = null; @@ -1744,38 +2105,44 @@ private static bool IsNetworkDropException(Exception error) return false; } + /// + /// The socket reported that its handshake failed, or the connect-attempt timer fired. + /// + /// + /// The failure belongs to the transition that opened the socket. If that transition still + /// owns the connection, this is where it continues: the reconnect loop is started under it, + /// unless it is already running - each failed attempt of the loop reaches here too. If a later + /// transition owns the connection, that operation is handling the connection now; this + /// callback closes its socket, clears what was its own, and does not sweep, report or + /// reconnect against a connection that is no longer this socket's. + /// private async Task OnConnectionFailed( Exception error, WebSocketClient? errorSocket = null, - long sessionId = 0, - bool isPingTimeoutReconnect = false, - bool isNetworkDropReconnect = false) + long sessionId = 0) { - // If this is a late callback from the socket closed due to ping timeout, ignore it - // (but not the initial call from ping handler which has isPingTimeoutReconnect=true) - if (_pingTimeoutSocket != null && _pingTimeoutSocket == errorSocket && !isPingTimeoutReconnect) + // A late callback from a socket the ping check or a network drop already retired. + if (_pingTimeoutSocket != null && _pingTimeoutSocket == errorSocket) { return; } - // If this is a late callback from the socket closed due to network drop, ignore it - // (but not the initial call which has isNetworkDropReconnect=true) - if (_networkDropSocket != null && _networkDropSocket == errorSocket && !isNetworkDropReconnect) + if (_networkDropSocket != null && _networkDropSocket == errorSocket) { return; } // Detect network drop via socket's FailureReason or exception type - var isNetworkDrop = isNetworkDropReconnect || - IsNetworkDropException(error) || - (errorSocket?.FailureReason == SocketFailureReason.NetworkDrop); + bool isNetworkDrop = IsNetworkDropException(error) || + errorSocket?.FailureReason == SocketFailureReason.NetworkDrop; - var currentUserInitiatedSocket = Volatile.Read(ref _userInitiatedSocket); + WebSocketClient? currentUserInitiatedSocket = Volatile.Read(ref _userInitiatedSocket); bool userInitiated; bool intentionalDisconnect; bool wasOpen; bool isCurrentSocket; - var isRetiringSession = false; + bool isRetiringSession = false; + ConnectionSession? failedSession = null; if (errorSocket != null) { @@ -1789,6 +2156,7 @@ private async Task OnConnectionFailed( if (_activeSession.SessionId == sessionId) { // Same session - check if marked as retiring + failedSession = _activeSession; isRetiringSession = _activeSession.IsRetiring; } else @@ -1800,8 +2168,8 @@ private async Task OnConnectionFailed( } else { - // Fallback for callbacks without session ID (timer timeout) - var activeSession = _activeSession; + // Fallback for callbacks without session ID + ConnectionSession? activeSession = _activeSession; if (activeSession != null) { if (activeSession.Socket != errorSocket) @@ -1812,11 +2180,21 @@ private async Task OnConnectionFailed( { isRetiringSession = true; } + else + { + failedSession = activeSession; + } } } } - isCurrentSocket = ws == errorSocket; + bool wsIsNull; + lock (_transitionLock) + { + isCurrentSocket = ReferenceEquals(ws, errorSocket); + wsIsNull = ws == null; + } + userInitiated = currentUserInitiatedSocket == errorSocket || IsSocketUserInitiated(errorSocket); intentionalDisconnect = _isIntentionalDisconnect || userInitiated || isRetiringSession; wasOpen = errorSocket.State == WebSocketState.Open; @@ -1828,7 +2206,7 @@ private async Task OnConnectionFailed( Interlocked.CompareExchange(ref _userInitiatedSocket, value: null, errorSocket); // For stale sockets (not current) or retiring sessions, do minimal cleanup - if ((!isCurrentSocket && ws != null) || isRetiringSession) + if ((!isCurrentSocket && !wsIsNull) || isRetiringSession) { // This is a late callback from an old socket - don't touch current connection if (intentionalDisconnect) @@ -1850,12 +2228,12 @@ private async Task OnConnectionFailed( timer?.Dispose(); timer = null; - // Use CloseSocketIntentionally for intentional disconnect, ping timeout, or network drop - // to suppress Critical error logging in WebSocketClient receive loop - if (intentionalDisconnect || isPingTimeoutReconnect || isNetworkDrop) + // Use CloseSocketIntentionally for intentional disconnect or network drop to suppress + // Critical error logging in WebSocketClient receive loop + if (intentionalDisconnect || isNetworkDrop) { // Track network drop socket for filtering late callbacks - if (isNetworkDrop && !isPingTimeoutReconnect && !intentionalDisconnect) + if (isNetworkDrop && !intentionalDisconnect) { _networkDropSocket = errorSocket; } @@ -1868,14 +2246,18 @@ private async Task OnConnectionFailed( errorSocket.Disconnect(); } - if (isCurrentSocket) + // Conditional and under the lock: a takeover during the calls above may have + // installed a socket of its own, and that one is not this callback's to clear. + lock (_transitionLock) { - ws = null; + if (ReferenceEquals(ws, errorSocket)) + { + ws = null; + } } } else { - isCurrentSocket = true; // null errorSocket means operate on current ws intentionalDisconnect = _isIntentionalDisconnect || currentUserInitiatedSocket != null; wasOpen = false; @@ -1885,10 +2267,19 @@ private async Task OnConnectionFailed( timer = null; // For null errorSocket with intentional disconnect, still need to clean up ws reference - if (intentionalDisconnect && ws != null) + if (intentionalDisconnect) { - CloseSocketIntentionally(ws); - ws = null; + WebSocketClient? current; + lock (_transitionLock) + { + current = ws; + ws = null; + } + + if (current != null) + { + CloseSocketIntentionally(current); + } } } @@ -1901,10 +2292,20 @@ private async Task OnConnectionFailed( return; } - // Reject awaiting connection requests and pending requests - // For ping timeout and network drop, use cancellation (no Critical logging in consuming apps) - // For other failures, use exception with message - if (isPingTimeoutReconnect || isNetworkDrop) + // From here on everything is about the connection as a whole - the sweep, the state, the + // loop - and that belongs to whoever owns it. A callback without a session (no caller in + // this class produces one; the parameter defaults exist for a null socket) is taken to be + // about the current transition. + long generation = failedSession?.Generation ?? CurrentGeneration(); + if (!Owns(generation)) + { + return; + } + + // Reject awaiting connection requests and pending requests. For a network drop, use + // cancellation (no Critical logging in consuming apps); for other failures, use an + // exception with the message. + if (isNetworkDrop) { requestManager.RejectAllWithCancellation(); connectionManager.RejectAllAwaitingWithCancellation(); @@ -1914,77 +2315,73 @@ private async Task OnConnectionFailed( connectionManager.RejectAllAwaiting(new NotConnectedException(error.Message)); } - // For ping timeout or network drop, use Warning severity and RestoringConnection state - // For other failures, use Error severity and Disconnected state - if (isPingTimeoutReconnect) + if (isNetworkDrop) { SetConnectionState( XrpConnectionState.RestoringConnection, - message: "Ping failed. Reconnecting...", + message: "Network connection lost. Reconnecting...", ConnectionCloseSeverity.Warning, reconnect: BuildReconnectInfo()); } - else if (isNetworkDrop) + else if (failedSession?.IsOpened == true) { + // The session had opened, so this is a connection that was lost, not one that never + // came up - whatever the socket's State says by now (Aborted, usually). Nothing in + // this class reports an established connection's failure this way any more (its + // receive loop reports a close), but the wording must not depend on that. SetConnectionState( XrpConnectionState.RestoringConnection, - message: "Network connection lost. Reconnecting...", + $"Connection lost: {error.Message}. Reconnecting...", + ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo()); + } + else if (IsReconnectActive()) + { + // During reconnect, use RestoringConnection with ReconnectInfo and Warning severity + SetConnectionState( + XrpConnectionState.RestoringConnection, + $"Connection attempt failed: {error.Message}", ConnectionCloseSeverity.Warning, reconnect: BuildReconnectInfo()); } else { - // Check if we're in a reconnect flow using the authoritative _reconnectMode flag - // This is set before any reconnect starts and cleared only when connection is stable - if (IsReconnectActive()) - { - // During reconnect, use RestoringConnection with ReconnectInfo and Warning severity - var reconnectErrorMessage = $"Connection attempt failed: {error.Message}"; - SetConnectionState( - XrpConnectionState.RestoringConnection, - reconnectErrorMessage, - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); - } - else - { - // True initial connection failure - no reconnect in progress - var errorMessage = $"Initial connection failed: {error.Message}"; - SetConnectionState(XrpConnectionState.Disconnected, errorMessage, ConnectionCloseSeverity.Error); - } + // True initial connection failure - no reconnect in progress + SetConnectionState( + XrpConnectionState.Disconnected, + $"Initial connection failed: {error.Message}", + ConnectionCloseSeverity.Error); } - // Start reconnect for initial connection failures, ping timeout, or network drop - // For ping timeout/network drop, wasOpen=true but we still need to reconnect - if (!wasOpen || isPingTimeoutReconnect || isNetworkDrop) + // Start reconnect for initial connection failures and network drops. For a network drop + // wasOpen is true, and the client still needs to reconnect. + if (!wasOpen || isNetworkDrop) { if (OnDisconnect is not null) { - // For ping timeout and network drop, use neutral message to avoid Critical logging - // in consuming apps that log OnDisconnect messages as errors - var disconnectMessage = isPingTimeoutReconnect - ? "Connection lost, reconnecting..." - : isNetworkDrop - ? "Network connection lost, reconnecting..." - : error.Message; + // For a network drop, use a neutral message to avoid Critical logging in + // consuming apps that log OnDisconnect messages as errors + string disconnectMessage = isNetworkDrop + ? "Network connection lost, reconnecting..." + : error.Message; await OnDisconnect?.Invoke(code: null, disconnectMessage)!; } - // Only start reconnect loop if not already running - // This prevents _reconnectAttempts from being reset when OnConnectionFailed - // is called from within the reconnect loop (each failed attempt triggers this callback) - // Check both _reconnectMode AND actual loop task status for accuracy - var loopIsRunning = _reconnectLoop != null && !_reconnectLoop.IsCompleted; - if (!loopIsRunning) - { - StartReconnectLoop(); - } + // Under the transition that opened the failed socket, unless a loop is already running + // for it - this callback runs for each failed attempt of that loop too, and restarting + // it would reset its counter - or a later transition took over during the callback. + StartReconnectLoop(generation); } } /// - /// Sends a message through the WebSocket connection. + /// Sends a message through the WebSocket connection, fire-and-forget. /// + /// + /// Kept for callers outside this class. and + /// use instead, which pairs the socket read with the send under + /// the retirement lock and observes the send. + /// /// The WebSocket client to send through. /// The message to send. /// Thrown when the WebSocket connection is null or closed. @@ -1995,6 +2392,100 @@ public void WebsocketSendAsync(WebSocketClient ws, string message) ws.SendMessage(message); } + /// + /// Writes a request into the connection as it stands right now. + /// + /// + /// + /// The socket is read and the send started under , the lock + /// every retirement takes the socket out under. Reading the socket first and sending after, + /// with nothing in between, still left the few instructions of that "nothing" for a + /// retirement to land in: the sweep had rejected the request, and the request went out to a + /// server the client had left. Under the lock a retirement finds the request either not yet + /// sent - and the cleared socket refuses it - or already handed to the socket. + /// + /// + /// What runs under the lock is the send's synchronous prefix: up to the write being issued + /// to , or up to the wait for the socket's send lock when + /// another message holds it - see for the + /// residue that case leaves. No consumer code and no await. + /// + /// + /// The send, which faults if the message could not be written. + /// There is no open connection to send into. + private Task SendRequestAsync(string message) + { + byte[] payload = Encoding.UTF8.GetBytes(message); + + lock (_transitionLock) + { + WebSocketClient? socket = ws; + if (socket is not { State: WebSocketState.Open }) + { + throw new DisconnectedException("WebSocket connection was closed before request could be sent"); + } + + return socket.SendMessageAsync(payload); + } + } + + /// + /// Starts sending for the request , + /// rejecting the request instead of leaving it pending if the message could not be written. + /// + /// + /// + /// The connection can be retired between the connectivity check and this send, and the + /// socket can refuse the write. Either way the request never left, so it must not stay + /// pending until RequestTimeout: the rejection is what the caller's await of the promise + /// surfaces. A request the retirement sweep rejected first is left as the sweep left it - + /// ignores a promise that is already gone. + /// + /// + /// The send is observed, not awaited, by the request. The promise is what honours the + /// request's timeout and the caller's token; a send that stalls - a half-open connection + /// whose send buffer has filled - would otherwise hold the caller past both, and the health + /// check's own ping with it, so the dead socket would never be noticed. + /// + /// + private void SendOrReject(Guid requestId, string message) + { + Task send; + try + { + send = SendRequestAsync(message); + } + catch (Exception error) + { + // Nothing left the client - a cleared socket, or the socket refusing to start the + // send at all - so the request is rejected with what stopped it rather than thrown + // past the promise the caller is about to await. + requestManager.Reject(requestId, error); + return; + } + + if (send.IsCompletedSuccessfully) + { + return; + } + + _ = RejectOnSendFailureAsync(requestId, send); + } + + private async Task RejectOnSendFailureAsync(Guid requestId, Task send) + { + try + { + await send.ConfigureAwait(false); + } + catch (Exception error) + { + requestManager.Reject( + requestId, + new DisconnectedException($"The request could not be written to the WebSocket: {error.Message}", error)); + } + } + private async Task EnsureConnectionForRequest(RequestFailurePolicy? policyOverride = null, CancellationToken cancellationToken = default) { if (ShouldBeConnected()) @@ -2009,7 +2500,14 @@ private async Task EnsureConnectionForRequest(RequestFailurePolicy? policyOverri switch (policy) { case RequestFailurePolicy.ImmediateFail: - throw new NotConnectedException(); + // Said in words: since #178 this is the exception a request issued during a + // server switch gets at once, where it used to get a TimeoutException with + // "Timeout" in it, and a consumer classifying failures by message text needs + // something to recognise. + throw new NotConnectedException( + "The client is not connected to a server and the request was refused at once " + + "(RequestFailurePolicy.ImmediateFail). Call Connect() first, or use " + + "RequestFailurePolicy.WaitForConnection to have requests wait for the connection."); case RequestFailurePolicy.WaitForConnection: await WaitForConnectionAsync(cancellationToken: cancellationToken); @@ -2032,9 +2530,12 @@ private void CheckIfNotConnected() throw new NotConnectedException("Client has been disconnected. Call Connect() to reconnect."); } - // Connecting or RestoringConnection states indicate an active attempt even if ws is null - var isActiveState = _currentConnectionState == XrpConnectionState.Connecting || - _currentConnectionState == XrpConnectionState.RestoringConnection; + // Connecting or RestoringConnection say an attempt is under way even with ws null. So + // does Connected with ws null: a close is being processed - OnceClose takes the socket out + // before its first await and reports the state, and starts the loop, after its callbacks - + // and every path out of that reports either RestoringConnection or Disconnected. Only + // Disconnected means nothing is in progress. + var isActiveState = _currentConnectionState != XrpConnectionState.Disconnected; var noConnectionAttemptActive = ws == null && _reconnectCts == null && !isActiveState; if (noConnectionAttemptActive) { @@ -2059,17 +2560,7 @@ public async Task>> Request( await EnsureConnectionForRequest(policyOverride, cancellationToken); var _request = requestManager.CreateRequest(request, timeout: timeout ?? config.RequestTimeout, adminCredentials: GetAdminCredentials(), cancellationToken: cancellationToken); - try - { - WebsocketSendAsync(ws, _request.Message); - } - catch (Exception error) when (error is EncodingFormatException or DisconnectedException) - { - // The connection can be retired between the check above and this send. The request - // never left, so it must not stay pending until RequestTimeout: the rejection is what - // the await below surfaces to the caller. - requestManager.Reject(_request.Id, error); - } + SendOrReject(_request.Id, _request.Message); object resolved = await _request.Promise; return XrplResponse.From>(resolved); @@ -2084,17 +2575,7 @@ public async Task> GRequest( await EnsureConnectionForRequest(policyOverride, cancellationToken); var _request = requestManager.CreateGRequest(request, timeout: timeout ?? config.RequestTimeout, adminCredentials: GetAdminCredentials(), cancellationToken: cancellationToken); - try - { - WebsocketSendAsync(ws, _request.Message); - } - catch (Exception error) when (error is EncodingFormatException or DisconnectedException) - { - // The connection can be retired between the check above and this send. The request - // never left, so it must not stay pending until RequestTimeout: the rejection is what - // the await below surfaces to the caller. - requestManager.Reject(_request.Id, error); - } + SendOrReject(_request.Id, _request.Message); object resolved = await _request.Promise; return XrplResponse.From(resolved); @@ -2128,28 +2609,33 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) return; } - // A user Disconnect() that landed while this socket was still connecting has marked it and - // is closing it. Installing it here would undo the disconnect: ws restored, the - // intentional-disconnect tracking cleared below, and the close that follows read as a - // network drop that starts a reconnect loop. The session check above does not cover this - - // Disconnect() does not retire the session, because OnceClose is what announces a user - // disconnect - so the socket's own marks are the signal. - if (_permanentlyDisconnected || IsSocketUserInitiated(connectedSocket)) + lock (_transitionLock) { - return; - } + // The transition that opened this socket has been superseded: a later operation owns + // the connection, and it either took this socket already or - for a socket that + // finished its handshake after the takeover - leaves it to ConnectCoreAsync to close. + // Installing it here would undo that operation. The socket's own marks cover the + // same case for a user Disconnect(), which does not retire the session (OnceClose is + // what announces a user disconnect) but does mark the socket it takes. + if (openedSession.Generation != _generation || + _permanentlyDisconnected || + IsSocketUserInitiated(connectedSocket)) + { + return; + } - // Verify the connected socket matches current ws, or update ws if it was cleared - if (ws == null) - { - // Restore ws reference from the connected socket - ws = connectedSocket; - } - else if (ws != connectedSocket) - { - // This is a stale callback from an old socket, ignore it silently - // Don't touch the timer - it belongs to the new connection - return; + // Verify the connected socket matches current ws, or update ws if it was cleared + if (ws == null) + { + // Restore ws reference from the connected socket + ws = connectedSocket; + } + else if (!ReferenceEquals(ws, connectedSocket)) + { + // This is a stale callback from an old socket, ignore it silently + // Don't touch the timer - it belongs to the new connection + return; + } } // Only stop timer for current socket's callback @@ -2157,8 +2643,11 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) timer?.Dispose(); timer = null; - // Clear all reconnect state - connection is now stable - ClearReconnectState(); + // The connection is up. The reconnect loop, if this socket is its attempt, releases its + // own state once ConnectCoreAsync returns to it - under the lock, with a re-check that + // the socket is still open - so nothing here touches it. Only the mode flags are cleared. + _reconnectMode = ReconnectMode.None; + _isFastReconnectActive = false; // Reset all intentional disconnect tracking now that new connection succeeded // This is the safe place to clear these - old socket callbacks will have already @@ -2195,13 +2684,23 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) await OnConnected?.Invoke(); } + // The handler is consumer code, and a Disconnect() or ChangeServer from inside it has + // taken the connection over by now: ws is theirs (or nobody's), and reporting + // Connected on top of the Disconnected they reported - then starting a ping timer that + // nothing would ever stop - would be this path speaking for a connection it no longer + // owns. + if (!Owns(openedSession.Generation)) + { + return; + } + Interlocked.Exchange(ref _connectHandlerFailures, value: 0); SetConnectionState(XrpConnectionState.Connected, message: $"Connected {url}"); } catch (Exception error) { connectionManager.RejectAllAwaiting(error); - await OnConnectHandlerFailedAsync(connectedSocket, error); + await OnConnectHandlerFailedAsync(connectedSocket, openedSession, error); return; // Don't start ping timer if connection failed } @@ -2227,8 +2726,9 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) /// /// /// The socket whose handler threw. + /// The session that socket serves; its generation is the transition this path continues. /// The exception thrown by the handler. - private async Task OnConnectHandlerFailedAsync(WebSocketClient failedSocket, Exception error) + private async Task OnConnectHandlerFailedAsync(WebSocketClient failedSocket, ConnectionSession failedSession, Exception error) { var errorHandler = OnError; if (errorHandler is not null) @@ -2315,14 +2815,22 @@ await errorHandler // Cleared before the sweep below, for the reason given in ChangeServer (issue #177): the // socket is open, and a request issued from a rejected continuation would otherwise go into it. // Taken after the notification above on purpose: the check that comes with the clear is the - // one that sees what the consumer's handler did. + // one that sees what the consumer's handler did. The ping timer and the message processor + // go in the same critical section - they are this connection's, and a takeover that lands + // between the clear and their stop would otherwise have its own torn down. bool wasCurrentSocket; - lock (_disconnectLock) + (Task? task, CancellationTokenSource? cts) detachedProcessor = default; + lock (_transitionLock) { wasCurrentSocket = ReferenceEquals(ws, failedSocket); if (wasCurrentSocket) { ws = null; + StopPingTimerSync(); + lock (_messageProcessorLock) + { + detachedProcessor = DetachMessageProcessor(); + } } } @@ -2338,9 +2846,8 @@ await errorHandler return; } - StopPingTimerSync(); requestManager.RejectAllWithCancellation(); - await StopMessageProcessorAsync(); + await AwaitMessageProcessorExitAsync(detachedProcessor.task, detachedProcessor.cts); await WaitForPingToFinishAsync(); // The socket is deliberately NOT marked as user-initiated: OnceClose must treat this as a real @@ -2348,64 +2855,31 @@ await errorHandler failedSocket.Cancel(); failedSocket.Disconnect(); - // Take ownership of the reconnect state instead of asking "is a loop already running?". - // This method can run inside the reconnect loop's own attempt: that loop breaks as soon as the - // socket reports Open, which happens before the handler has even finished failing. Both this check - // and the one in OnceClose would then race with the loop's exit, and losing the race leaves nobody - // reconnecting - the very wedge this path exists to prevent. Cancel whatever is there, start fresh; - // the later OnceClose sees a live loop and correctly stands down. - // Seed the attempt counter with the consecutive-failure count. StopReconnectLoop zeroes - // _reconnectAttempts and a fresh sequence would zero it again, and CalcBackoff derives the - // delay from that counter alone — so without the seed every handler failure would restart - // the backoff at ReconnectBaseDelay. With StopAfterMaxAttempts = false (no give-up branch) - // that means connect -> handler failure -> teardown forever at a constant 2s, a sustained - // connection load on a node that accepts TCP but cannot serve requests yet. - RestartReconnectLoop(initialAttempts: failures); + // Continue the transition this socket belongs to. If its loop is running - this method can + // run inside the loop's own attempt, and OnceClose for the socket will ask the same + // question - the loop carries on with its own counter, which grows per attempt; the + // decision is one lock, so there is no exit to race with. Otherwise a loop starts here, + // seeded with the consecutive-failure count: a fresh sequence starts its counter at zero, + // and CalcBackoff derives the delay from that counter alone - so without the seed every + // handler failure would restart the backoff at ReconnectBaseDelay. With + // StopAfterMaxAttempts = false (no give-up branch) that means connect -> handler failure -> + // teardown forever at a constant 2s, a sustained connection load on a node that accepts + // TCP but cannot serve requests yet. + StartReconnectLoop(failedSession.Generation, initialAttempts: failures); } /// /// Whether is the one installed as the connection right now. Read - /// under _disconnectLock, the lock every retirement path clears ws under. + /// under , the lock every retirement path clears ws under. /// private bool IsCurrentSocket(WebSocketClient socket) { - lock (_disconnectLock) + lock (_transitionLock) { return ReferenceEquals(ws, socket); } } - /// - /// Retires the current reconnect session and installs a fresh one in a single transaction, - /// seeding the attempt counter with . - /// - /// - /// Doing this as StopReconnectLoop(); _reconnectLoop = null; StartReconnectLoop(seed); - /// took the lock twice with a bare write in between, so a concurrent start (from OnceClose or - /// OnConnectionFailed) could slip in and install its own loop; the seeded start would then see - /// a live loop, return without applying the seed, and the backoff would silently stop growing - /// across consecutive handler failures — the very regression the seed exists to prevent. - /// - private void RestartReconnectLoop(int initialAttempts) - { - CancellationTokenSource retired; - lock (_reconnectStateLock) - { - retired = _reconnectCts; - _reconnectMode = ReconnectMode.LoopReconnect; - _isFastReconnectActive = false; - _reconnectAttempts = initialAttempts; - _reconnectCts = new CancellationTokenSource(); - - // Safe to start under the lock: ReconnectLoopAsync reads its token and yields before - // anything else, so this only schedules the loop - no consumer notification runs inline. - _reconnectLoop = ReconnectLoopAsync(_reconnectCts); - } - - retired?.Cancel(); - retired?.Dispose(); - } - private async Task OnceClose(int? code, string? description, WebSocketClient closingSocket, long sessionId) { var (severity, userMessage) = DescribeClose(code, description); @@ -2442,8 +2916,13 @@ private async Task OnceClose(int? code, string? description, WebSocketClient clo } // Check if this is the current socket or a stale callback from an old socket - var isCurrentSocket = ws == closingSocket; - var wsWasNull = ws == null; + bool isCurrentSocket; + bool wsWasNull; + lock (_transitionLock) + { + isCurrentSocket = ReferenceEquals(ws, closingSocket); + wsWasNull = ws == null; + } var isUserInitiated = Interlocked.CompareExchange( ref _userInitiatedSocket, @@ -2470,35 +2949,56 @@ private async Task OnceClose(int? code, string? description, WebSocketClient clo return; } - // Only for the current socket - and the message processor goes with it, this connection - // is over. - StopPingTimerSync(); - await StopMessageProcessorAsync(); + // Only for the current socket - and the ping timer and the message processor go with it, + // this connection is over. Taken in one critical section with the clear of ws: a takeover + // that lands between them would otherwise have its own timer and processor torn down. The + // clear is conditional for the same reason - a takeover may already have installed a + // socket of its own, and that one is not this callback's to clear. + (Task? task, CancellationTokenSource? cts) detachedProcessor; + lock (_transitionLock) + { + StopPingTimerSync(); + lock (_messageProcessorLock) + { + detachedProcessor = DetachMessageProcessor(); + } + + if (ReferenceEquals(ws, closingSocket)) + { + ws = null; + } + } + + await AwaitMessageProcessorExitAsync(detachedProcessor.task, detachedProcessor.cts); // Check if this is a network drop (FailureReason set by WebSocketClient) var isNetworkDrop = closingSocket.FailureReason == SocketFailureReason.NetworkDrop; - + // Track network drop socket for immediate reconnect if (isNetworkDrop && !intentionalDisconnect) { _networkDropSocket = closingSocket; } - // For intentional disconnect or network drop, use cancellation (no Critical logging) - if (intentionalDisconnect || isNetworkDrop) - { - requestManager.RejectAllWithCancellation(); - } - else - { - requestManager.RejectAll( - new DisconnectedException($"websocket was closed, code: {code}, reason: {userMessage}")); - } - - // Clear ws reference - if (isCurrentSocket) + // The sweep, the state and the loop belong to whoever owns the connection. This socket's + // transition owns it unless a later one took over - a user Disconnect() that closed this + // socket, a Connect() or ChangeServer issued while it was closing - and that operation is + // sweeping and reporting for itself now; rejecting its requests here would reject the + // requests of the connection it is building. A callback with no session to name (none + // in this class) is taken to be about the current transition. + long closingGeneration = closingSession?.Generation ?? CurrentGeneration(); + if (Owns(closingGeneration)) { - ws = null; + // For intentional disconnect or network drop, use cancellation (no Critical logging) + if (intentionalDisconnect || isNetworkDrop) + { + requestManager.RejectAllWithCancellation(); + } + else + { + requestManager.RejectAll( + new DisconnectedException($"websocket was closed, code: {code}, reason: {userMessage}")); + } } CompleteDisconnectTcs(); @@ -2528,9 +3028,15 @@ await NotifySessionEndedAsync( intentionalDisconnect ? SessionEndReason.UserDisconnected : SessionEndReason.ConnectionLost, userMessage); + // Asked again after the two callbacks above: a handler may have moved the client on, and + // the state it reports is its own. + if (!Owns(closingGeneration)) + { + return; + } + if (intentionalDisconnect) { - _reconnectAttempts = 0; var noReconnectMessage = $"Connection closed permanently. {userMessage}"; SetConnectionState(XrpConnectionState.Disconnected, noReconnectMessage, ConnectionCloseSeverity.Warning); return; @@ -2538,163 +3044,143 @@ await NotifySessionEndedAsync( if (ShouldReconnect(code) || code == 1000) { - // Check if reconnect loop is already running - don't reset counter or start new loop - var loopIsRunning = _reconnectLoop != null && !_reconnectLoop.IsCompleted; - if (!loopIsRunning) + // The loop is started before the notification, so an escaping handler cannot cost it, + // and only when none is running for this transition - the decision is one lock, so + // there is no loop exit to race with. A loop that is running handles the close itself: + // it re-checks the socket under the same lock before it releases. + ReconnectInfo firstAttempt = BuildReconnectInfo(explicitAttempt: 1); + if (StartReconnectLoop(closingGeneration)) { - // Set _reconnectAttempts = 1 before notification so BuildReconnectInfo returns correct value - _reconnectAttempts = 1; SetConnectionState( XrpConnectionState.RestoringConnection, userMessage, severity, - reconnect: BuildReconnectInfo()); - StartReconnectLoop(); + reconnect: firstAttempt); } - // else: loop is already running and will handle reconnection, don't reset _reconnectAttempts } else { - _reconnectAttempts = 0; + lock (_transitionLock) + { + if (Owns(closingGeneration)) + { + _reconnectAttempts = 0; + } + } + var noReconnectMessage = $"Connection closed permanently. {userMessage}"; SetConnectionState(XrpConnectionState.Disconnected, noReconnectMessage, ConnectionCloseSeverity.Warning); } } - private void StopReconnectLoop() - { - // Detach under the lock, then cancel/dispose outside it: a start racing with this stop can - // no longer have its fresh source torn down, and cancellation callbacks never run while the - // lock is held. - CancellationTokenSource retired; - lock (_reconnectStateLock) - { - retired = _reconnectCts; - _reconnectCts = null; - _reconnectAttempts = 0; - - // Drop the task reference too, in the same transaction. The retired loop exits - // asynchronously - it only notices it lost ownership on its next check - so leaving the - // reference behind makes StartReconnectLoop see `!IsCompleted` and return without - // starting anything, while the retired loop then stands down on its ownership check. - // Nobody would be reconnecting. Reachable whenever Connect or ChangeServer stops a live - // loop and the new connection fails. - _reconnectLoop = null; - } - - retired?.Cancel(); - retired?.Dispose(); - // Note: Do NOT clear _reconnectMode here! - // _reconnectMode is cleared only by: - // - OnceOpen (connection succeeded) - // - End of ReconnectLoopAsync (loop terminated) - // - ClearReconnectState (user-initiated disconnect) - // This prevents race conditions where StopReconnectLoop is called during - // fast reconnect transitions (RetireCurrentSessionAndReconnectAsync) - _isFastReconnectActive = false; // Legacy flag for backward compatibility - } - - /// - /// Clears all reconnect state. Only called when connection is stable or user disconnects. - /// - private void ClearReconnectState() - { - StopReconnectLoop(); - _reconnectMode = ReconnectMode.None; - } - /// - /// Starts a reconnect loop unless one is already running, reusing a pre-created cancellation - /// source when there is one. A fresh sequence starts its attempt counter at zero, so the first - /// delay is CalcBackoff(1) — twice ReconnectBaseDelay — except on the - /// ping-timeout and network-drop paths, where the first attempt skips the delay entirely. The - /// OnConnected-handler path needs a seeded counter instead and uses - /// . + /// Starts the reconnect loop for , unless that transition no + /// longer owns the connection or its loop is already running. Reuses the cancellation source + /// the fast reconnect installed when there is one; a fresh sequence starts its attempt counter + /// at , so the first delay of a fresh sequence is + /// CalcBackoff(1) - twice ReconnectBaseDelay - except on the ping-timeout and + /// network-drop paths, where the first attempt skips the delay entirely. /// - private void StartReconnectLoop() + /// + /// The whole decision - does the transition still own the connection, is its loop already + /// running, is the current source reusable, install a fresh one, hand it to the new loop - is + /// one critical section under , the same lock the loop releases + /// itself under. Split, it would race with that release and with another start: two loops + /// could end up running, or none. + /// + /// Whether a loop was started. + private bool StartReconnectLoop(long generation, int initialAttempts = 0) { - // Set reconnect mode to LoopReconnect (upgrades from FastReconnect or sets from None) - _reconnectMode = ReconnectMode.LoopReconnect; - - // The whole decision — is a loop already running, is the current source reusable, install a - // fresh one, hand it to the new loop — is one transaction. Split across the lock it would - // race with StopReconnectLoop and with another start: two loops could end up running, or a - // loop could be handed a source that a concurrent stop has already disposed. CancellationTokenSource retired = null; - lock (_reconnectStateLock) + lock (_transitionLock) { - // CRITICAL: If a loop is already running, don't start another or reset the counter - // This prevents _reconnectAttempts from being reset mid-loop when callbacks trigger - // reconnect logic (OnceClose, OnConnectionFailed, etc.) - var loopIsRunning = _reconnectLoop != null && !_reconnectLoop.IsCompleted; - if (loopIsRunning) + if (_generation != generation || _reconnectLoopGeneration == generation) { - // Loop is already running - let it continue, don't reset _reconnectAttempts - return; + return false; } - // If we have a valid pre-created CTS (from RetireCurrentSessionAndReconnectAsync), - // we should reuse it. Check for this case first. - var existingCts = _reconnectCts; - var hasValidPreCreatedCts = existingCts != null && !existingCts.IsCancellationRequested; + // Set reconnect mode to LoopReconnect (upgrades from FastReconnect or sets from None) + _reconnectMode = ReconnectMode.LoopReconnect; - // If no valid pre-created CTS, create a new one - // Only reset _reconnectAttempts when creating a FRESH CTS (new reconnect sequence) - if (!hasValidPreCreatedCts) + // A valid pre-created source (from the fast reconnect) is reused, and the sequence it + // belongs to continues with its counter - the seed only ever raises it. Otherwise a + // fresh sequence starts on a fresh source. + CancellationTokenSource existingCts = _reconnectCts; + bool hasValidPreCreatedCts = existingCts != null && !existingCts.IsCancellationRequested; + if (hasValidPreCreatedCts) { - // Retire the old CTS after the lock is released - see _reconnectStateLock + _reconnectAttempts = Math.Max(_reconnectAttempts, initialAttempts); + } + else + { + // Retire the old source after the lock is released - see _transitionLock retired = existingCts; _reconnectCts = new CancellationTokenSource(); - _reconnectAttempts = 0; + _reconnectAttempts = initialAttempts; } - // else: Reuse existing valid CTS (pre-created for fast reconnect) - // Don't reset _reconnectAttempts - this is continuation of existing reconnect sequence - // Note: _reconnectLoop was already cleared by RetireCurrentSessionAndReconnectAsync - // Safe to start under the lock: ReconnectLoopAsync yields before touching anything, so - // this call only schedules the loop and returns - no consumer notification runs inline. - _reconnectLoop = ReconnectLoopAsync(_reconnectCts); + _reconnectLoopGeneration = generation; + + // Safe to start under the lock: ReconnectLoopAsync reads its token and yields before + // anything else, so this only schedules the loop - no consumer notification runs inline. + _ = ReconnectLoopAsync(generation, _reconnectCts); } retired?.Cancel(); retired?.Dispose(); + return true; + } + + /// + /// Releases the loop's claim on the reconnect state, for a loop that connected. Must be called + /// with held, by a loop that still owns its generation. The + /// source comes out for the caller to dispose outside the lock. + /// + private CancellationTokenSource? ReleaseReconnectLoopLocked(CancellationTokenSource ownCts) + { + _reconnectLoopGeneration = 0; + _reconnectAttempts = 0; + + if (!ReferenceEquals(_reconnectCts, ownCts)) + { + return null; + } + + _reconnectCts = null; + return ownCts; } - private async Task ReconnectLoopAsync(CancellationTokenSource ownCts) + private async Task ReconnectLoopAsync(long generation, CancellationTokenSource ownCts) { - // The CTS this loop owns. StopReconnectLoop cancels without awaiting the loop, so a retired loop - // can still be running - or reach its tail - after a replacement has been installed. Everything - // this loop writes to shared reconnect state is therefore guarded by an ownership check. + // The source this loop runs on. A takeover cancels it without awaiting the loop, so a + // retired loop can still be running - or reach its tail - after another transition has + // begun. Everything this loop writes to shared state is therefore guarded by an ownership + // check on its generation, under the lock. // // Read BEFORE the yield below, and deliberately so: the caller still holds - // _reconnectStateLock here, so this source cannot yet have been retired. After the yield a - // concurrent stop may already have disposed it - Cancel/Dispose of a retired source run + // _transitionLock here, so this source cannot yet have been retired. After the yield a + // concurrent takeover may already have disposed it - Cancel/Dispose of a retired source run // outside the lock - and CancellationTokenSource.Token throws ObjectDisposedException once // disposed. Taken after the yield, that throw would land outside every try below, faulting // the loop before its first attempt and vanishing as an unobserved task exception. CancellationToken ct = ownCts.Token; // Yield so nothing beyond that read runs inline on the caller: StartReconnectLoop starts the - // loop while holding _reconnectStateLock, and a consumer notification executing under that + // loop while holding _transitionLock, and a consumer notification executing under that // lock could deadlock against any path that takes it (Disconnect from a handler, say). await Task.Yield(); - // Don't reset _reconnectAttempts here - it may be pre-set to 1 by fast reconnect path - // StartReconnectLoop() sets it to 0 when creating a new CTS - // Clear fast reconnect flag - reconnect loop has taken ownership - // This must happen AFTER _reconnectCts is valid (which StartReconnectLoop ensures) - // so any pending OnConnectionFailed callbacks still see IsReconnectActive()=true via CTS _isFastReconnectActive = false; - + // For ping timeout or network drop, first attempt should be immediate (no delay) var isImmediateReconnect = _pingTimeoutSocket != null || _networkDropSocket != null; while (!ct.IsCancellationRequested) { - if (!ReferenceEquals(_reconnectCts, ownCts)) + if (!Owns(generation)) { - // Retired: a newer loop owns the reconnect sequence now. + // Superseded: a later transition owns the connection now. break; } @@ -2703,9 +3189,9 @@ private async Task ReconnectLoopAsync(CancellationTokenSource ownCts) // Skip delay for first attempt if this is immediate reconnect (ping timeout or network drop) var skipDelay = isImmediateReconnect && _reconnectAttempts == 1; isImmediateReconnect = false; // Only affects first attempt - + var delay = skipDelay ? TimeSpan.Zero : CalcBackoff(_reconnectAttempts); - var reconnectMessage = skipDelay + var reconnectMessage = skipDelay ? "Reconnecting immediately..." : $"Reconnecting in {delay.TotalSeconds:F1} seconds... (attempt #{_reconnectAttempts})"; var type = ConnectionCloseSeverity.Info; @@ -2743,57 +3229,54 @@ private async Task ReconnectLoopAsync(CancellationTokenSource ownCts) } catch (ObjectDisposedException) { - // The source this loop owns was retired and disposed while the delay was being - // set up: registering a callback on a token whose source is gone throws instead - // of cancelling. Same meaning as cancellation - a newer sequence owns the - // reconnect state now - so leave quietly rather than fault the task. + // The source this loop runs on was retired and disposed while the delay was + // being set up: registering a callback on a token whose source is gone throws + // instead of cancelling. Same meaning as cancellation - a later transition owns + // the connection now - so leave quietly rather than fault the task. break; } } - if (ct.IsCancellationRequested) + if (ct.IsCancellationRequested || !Owns(generation)) { break; } try { - // ===================================================== - // SESSION ISOLATION (same as ChangeServer) - // ===================================================== - // Somebody else connected while this loop was waiting out its delay - another - // attempt on the same source, or the fast-reconnect path. Whatever is in ws is live, - // and retiring it below would trade a healthy connection for a reconnect nobody - // needed. Every path that starts this loop does so because the connection is gone, - // so an open socket here always belongs to someone who got there first. - if (IsConnected()) - { - lock (_reconnectStateLock) + // Retire the previous attempt's session and socket - under the lock, without a + // takeover: this is the same transition, one attempt further on. An open socket + // here means the transition connected by some other means while this loop was + // waiting out its delay; the loop is done then, and retiring a healthy connection + // would trade it for a reconnect nobody needed. + ConnectionSession? oldSession = null; + WebSocketClient? oldSocket = null; + CancellationTokenSource? releasedBeforeAttempt = null; + bool alreadyConnected; + lock (_transitionLock) + { + if (!Owns(generation)) { - if (ReferenceEquals(_reconnectCts, ownCts)) - { - _reconnectAttempts = 0; - } + break; } - break; + alreadyConnected = ShouldBeConnected(); + if (alreadyConnected) + { + releasedBeforeAttempt = ReleaseReconnectLoopLocked(ownCts); + } + else + { + DetachLocked(retireSession: true, out oldSession, out oldSocket); + } } - // Mark old session as retiring before creating new connection - // so late callbacks from old socket are properly ignored. - ConnectionSession? oldSession; - WebSocketClient? oldSocket; - lock (_sessionLock) + if (alreadyConnected) { - oldSession = _activeSession; - oldSession?.MarkAsRetiring(); - } - lock (_disconnectLock) - { - oldSocket = ws; - ws = null; + releasedBeforeAttempt?.Dispose(); + return; } - + // Mark old socket for intentional disconnect (per-socket tracking) if (oldSocket != null) { @@ -2803,36 +3286,54 @@ private async Task ReconnectLoopAsync(CancellationTokenSource ownCts) _ = RetireOldSessionAsync(oldSession, oldSocket); } - await ConnectInternalAsync(ct); - - if (IsConnected()) + await ConnectCoreAsync(generation, ct); + + // Release under the lock, with the socket re-checked under the same lock. This is + // the window OnceClose used to fall into: the loop saw an open socket, broke out, + // and a close processed before its task completed saw a running loop that was + // about to exit and started nothing. Now a close either sees the loop released + // (and starts a new one) or sees it running - and the loop, taking the lock next, + // sees the socket closed and goes round again. + CancellationTokenSource? released = null; + bool settled; + lock (_transitionLock) { - // Ownership check and the write it guards belong together: checked outside the - // lock, this loop could be retired in between and reset a live sequence's counter. - lock (_reconnectStateLock) + if (!Owns(generation)) { - if (ReferenceEquals(_reconnectCts, ownCts)) - { - _reconnectAttempts = 0; - } + break; } - break; + settled = ShouldBeConnected(); + if (settled) + { + released = ReleaseReconnectLoopLocked(ownCts); + } + } + + if (settled) + { + released?.Dispose(); + return; } } catch (OperationCanceledException) { - // Reconnect loop was cancelled (e.g., by ChangeServer or StopReconnectLoop) - // Exit the loop quietly without logging an error + // Cancelled by a takeover - ChangeServer, Connect, Disconnect. Exit quietly. Debug.WriteLine($"{DateTime.Now}Reconnect loop cancelled"); break; } catch (Exception ex) { + // A later transition owns the connection: its state is its own to report. + if (!Owns(generation)) + { + break; + } + // For network exceptions, use Warning severity to avoid Critical logging in consuming apps var isNetworkError = IsNetworkDropException(ex); var severity = isNetworkError ? ConnectionCloseSeverity.Warning : ConnectionCloseSeverity.Error; - var errorMessage = isNetworkError + var errorMessage = isNetworkError ? $"Reconnection attempt #{_reconnectAttempts}: network unavailable" : $"Reconnection attempt #{_reconnectAttempts} failed: {ex.Message}"; SetConnectionState( @@ -2847,37 +3348,35 @@ private async Task ReconnectLoopAsync(CancellationTokenSource ownCts) // This ensures late callbacks from ping-timeout socket are still filtered // even if reconnect attempts fail - // A newer loop may already have taken over (this one was retired by StopReconnectLoop, which does - // not await it). Its state belongs to that loop: clearing the mode or disposing the CTS here would - // strand the live reconnect sequence. - if (!ReferenceEquals(_reconnectCts, ownCts)) + // Exited without connecting: cancelled, or out of attempts. If a later transition owns the + // connection, its state is its own - the takeover that superseded this loop cleared the + // loop's claim as part of taking over. Otherwise the claim is released here, and the + // source is disposed when the sequence is over for good. + CancellationTokenSource finished = null; + lock (_transitionLock) { - return; - } + if (!Owns(generation) || _reconnectLoopGeneration != generation) + { + return; + } - // When loop exits (cancelled, max attempts, or success) and connection is not established, - // clear the reconnect mode. If connected, OnceOpen already cleared it. - if (!IsConnected()) - { - _reconnectMode = ReconnectMode.None; - } + _reconnectLoopGeneration = 0; - if (config.StopAfterMaxAttempts && _reconnectAttempts >= config.MaxReconnectAttempts) - { - // Re-check ownership inside the lock: between the check above and here a new sequence - // could have installed its own source, and disposing that one would strand it. - CancellationTokenSource finished = null; - lock (_reconnectStateLock) + if (!ShouldBeConnected()) { - if (ReferenceEquals(_reconnectCts, ownCts)) - { - finished = _reconnectCts; - _reconnectCts = null; - } + _reconnectMode = ReconnectMode.None; } - finished?.Dispose(); + if (config.StopAfterMaxAttempts && + _reconnectAttempts >= config.MaxReconnectAttempts && + ReferenceEquals(_reconnectCts, ownCts)) + { + finished = _reconnectCts; + _reconnectCts = null; + } } + + finished?.Dispose(); } private volatile int _pingRunning = 0; @@ -2952,7 +3451,7 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts) } WebSocketClient? currentSocket; - lock (_disconnectLock) + lock (_transitionLock) { currentSocket = ws; } @@ -2973,8 +3472,8 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts) if (!IsConnected()) { Debug.WriteLine($"{DateTime.Now}[PING-CHECK] Not connected (State={State()}), triggering reconnect"); - _pingTimeoutSocket = ws; - await RetireCurrentSessionAndReconnectAsync($"Ping detected disconnected state ({State()})."); + _pingTimeoutSocket = currentSocket; + await RetireCurrentSessionAndReconnectAsync($"Ping detected disconnected state ({State()}).", currentSocket); return; } @@ -2992,10 +3491,11 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts) double inactivityLimit = config.InactivityTimeout.TotalSeconds; if (timeSinceLastActivity > inactivityLimit) { - _pingTimeoutSocket = ws; + _pingTimeoutSocket = currentSocket; await RetireCurrentSessionAndReconnectAsync( - $"Connection timeout (no activity for {inactivityLimit:F0}+ seconds)."); + $"Connection timeout (no activity for {inactivityLimit:F0}+ seconds).", + currentSocket); return; } @@ -3068,9 +3568,9 @@ await Request( Debug.WriteLine($"{DateTime.Now}Ping request error: {pingEx.Message}"); - _pingTimeoutSocket = ws; + _pingTimeoutSocket = currentSocket; - await RetireCurrentSessionAndReconnectAsync("Ping failed."); + await RetireCurrentSessionAndReconnectAsync("Ping failed.", currentSocket); return; } } @@ -3447,20 +3947,6 @@ private void StartMessageProcessor() } } - /// - /// Stops the background message processor and disposes resources. - /// - private Task StopMessageProcessorAsync() - { - (Task? task, CancellationTokenSource? cts) detached; - lock (_messageProcessorLock) - { - detached = DetachMessageProcessor(); - } - - return AwaitMessageProcessorExitAsync(detached.task, detached.cts); - } - /// /// Takes the processor's channel, source and task out of their fields, completes the channel /// and cancels the source, so the reader exits on its own. Must be called with @@ -3890,7 +4376,7 @@ string Text() /// /// Matching the id is not enough: ChangeServer and the reconnect loop call /// MarkAsRetiring() on the session while it is still _activeSession, and only - /// ConnectInternalAsync installs its replacement. Frames arriving in that window carry + /// ConnectCoreAsync installs its replacement. Frames arriving in that window carry /// the id of the very session being retired, so the retiring flag is part of the test - as /// OnceOpen and the other lifecycle guards do it, and under the same lock, since /// IsRetiring is a plain bool published only by _sessionLock. diff --git a/Xrpl/Xrpl.csproj b/Xrpl/Xrpl.csproj index 41014b18..0c70dc24 100644 --- a/Xrpl/Xrpl.csproj +++ b/Xrpl/Xrpl.csproj @@ -14,7 +14,7 @@ Apache-2.0 https://github.com/StaticBit-io/XrplCSharp XrplCSharp - 11.3.2.0 + 11.4.0.0 diff --git a/specs/2026-09-06-connection-transition-owner-design.md b/specs/2026-09-06-connection-transition-owner-design.md new file mode 100644 index 00000000..febc862d --- /dev/null +++ b/specs/2026-09-06-connection-transition-owner-design.md @@ -0,0 +1,233 @@ +# Владелец перехода соединения (issue #179) + +Дата: 2026-09-06. Статус: реализован, ветка `claude/connection-transition-owner-691f22`. + +> Cold-review (три прохода, шесть ревьюеров: sonnet, opus, fable) нашёл ещё девять дефектов на +> тех же путях, четыре из них воспроизводимы на базовом коде; все исправлены в этом же изменении, +> потому что оно переписывает код, где они живут: `OnceOpen` после `await OnConnected` без +> проверки владения; `_isIntentionalDisconnect`, который `Connect()` после `Disconnect()` не +> сбрасывал; таймер попытки, живущий после тихо отменённого handshake; `Connect()` поверх +> закрывающегося сокета без объявления конца сессии и без sweep'а запросов; потерянный +> `OnSessionEnded` при перекрытии `ChangeServer`/fast reconnect до объявления и при +> `Disconnect()`+`Connect()` подряд (объявляет теперь сам `Disconnect()`); вторая серия попыток +> fast reconnect после `StopAfterMaxAttempts`; незавершаемый `_disconnectTcs` для сокета в +> handshake. Отправка запроса *наблюдается*, а не ожидается перед promise — иначе зависшая запись +> держала бы вызывающего дольше его таймаута. Правило `DetachLocked`: без сокета сессия не +> забирается — она принадлежит тому, кто забрал сокет. + +Issue: https://github.com/StaticBit-io/XrplCSharp/issues/179 — продолжение #178. Четыре гонки +между конкурентными операциями над соединением и два хвоста (`NotConnectedException` без +сообщения, `WebSocketClient.SendMessageAsync` переподключает сокет при отправке). + +## 1. Проблема + +В `Xrpl/Client/connection.cs` соединение двигают шесть путей: `ChangeServer`, +`Disconnect`/`DisconnectAndWaitAsync`, `Connect`, `RetireCurrentSessionAndReconnectAsync` +(быстрое переподключение из ping-проверки), `ReconnectLoopAsync` и +`OnConnectHandlerFailedAsync`. Каждый сам решает, что делать с сокетом, а два одновременно +идущих пути согласуются точечными проверками `ReferenceEquals(ws, …)` после того `await` +или consumer-callback'а, который кто-то заметил. #178 добавил три такие проверки, ревью +каждый раз находило следующее окно. + +Оставшиеся окна (нумерация как в issue): + +1. **Выход `ReconnectLoopAsync` против `OnceClose`.** Цикл делает `break` по `IsConnected()`, + пока его задача ещё завершается; закрытие, обработанное в этот зазор, видит + `_reconnectLoop` «живым», не запускает замену — и никто не переподключается. +2. **Отправка против retirement.** `Request`/`GRequest` читают `ws` и вызывают `SendMessage`; + между чтением и отправкой соединение может быть retired. Promise уже отклонён, но запрос + всё же уходит на сервер, который клиент покинул. +3. **Retire против блокирующего consumer'а.** `RetireCurrentSessionAndReconnectAsync` + захватывает сессию и сокет *после* `SetConnectionState(RestoringConnection)`. Handler, + который внутри этого callback'а успел установить новую сессию, получает её помеченной + retiring. +4. **`ChangeServer` перекрывает конкурентный `Disconnect()`.** `ChangeServer` уступает поток + (остановка процессора, уведомление о конце сессии, ожидание ping); `Disconnect()` в этот + момент находит `ws == null`, выставляет `_permanentlyDisconnected` и возвращает «already + disconnected»; `ChangeServer` затем сбрасывает флаг и подключается — клиент онлайн после + того, как пользователь его выключил. + +Хвосты: + +5. `EnsureConnectionForRequest` при `ImmediateFail` бросает `new NotConnectedException()` — + `Message` равен runtime-умолчанию, consumer, классифицирующий по тексту, его не узнаёт. +6. `WebSocketClient.SendMessageAsync` на не-Open сокете вызывает `_ = Connect()` — + `ConnectAsync` на уже использованном `ClientWebSocket` бросает, `_ws` уничтожается, + поднимается `OnConnectionError` — и всё равно продолжает `SendAsync`. Отправка не должна + переподключать; отправка на неоткрытый сокет должна провалиться, и провал должен быть + наблюдаем владельцем запроса. + +## 2. Решение: поколение перехода + +### 2.1. Инвариант + +У соединения в каждый момент ровно один владелец — **переход** (transition), обозначенный +монотонным `long _generation`. Кто владеет поколением, тот и решает, что происходит с +сокетом. Операция, обнаружившая, что поколение сменилось, **отступает** (stands down): ничего +больше не трогает и, если у неё есть вызывающий, сообщает ему, что её перекрыли. + +Все записи в `ws`, `_generation`, `_permanentlyDisconnected` и состояние reconnect-цикла +(`_reconnectCts`, `_reconnectLoopGeneration`, `_reconnectAttempts`) делаются под одной +блокировкой `_transitionLock` (она поглощает нынешние `_disconnectLock` и +`_reconnectStateLock`). `_sessionLock` остаётся отдельным (его берёт per-frame путь +`IsFromLiveSession`) и вкладывается внутрь `_transitionLock`; `_messageProcessorLock` тоже +вкладывается. Порядок захвата фиксирован: `_transitionLock` → `_sessionLock` → +`_messageProcessorLock`. Под `_transitionLock` не выполняется consumer-код и нет `await`. + +### 2.2. Кто начинает поколение, кто продолжает + +**Начинают новое поколение** (`TakeOver`) — команды пользователя и быстрый reconnect: + +| Путь | Условие захвата | Что делает под блокировкой | +|---|---|---| +| `Connect()` | безусловно, если не `IsConnected()` | `++_generation`, снять `_permanentlyDisconnected`, остановить reconnect-цикл (cts наружу для Cancel/Dispose), `ws` не трогает (он `null` или Connecting: Connecting-сокет предыдущего владельца забирается и закрывается) | +| `ChangeServer()` | безусловно | `++_generation`, retire сессии, забрать `ws`, остановить цикл, остановить ping-таймер, отсоединить процессор сообщений | +| `Disconnect()` / `DisconnectAndWaitAsync()` | безусловно | `++_generation`, `_permanentlyDisconnected = true`, забрать `ws` (сессия **не** retire — `OnceClose` объявляет `UserDisconnected`), остановить цикл, ping, процессор | +| `RetireCurrentSessionAndReconnectAsync(socket)` | **условно**: только если `ws` всё ещё тот сокет, который ping-проверка признала мёртвым | как `ChangeServer`, плюс установить `_reconnectCts`, `_reconnectAttempts = 1` | + +Условный захват закрывает ещё одну гонку, которую issue не перечисляет: `Disconnect()` во +время ping-проверки. Раньше `RetireCurrent…` проверял `_permanentlyDisconnected` в двух местах +по ходу; теперь он просто не получает владения, если сокет уже забрали. + +**Продолжают текущее поколение** (никогда не увеличивают его) — callback'и сокета и цикл: + +- `OnceClose` / `OnConnectionFailed` для сокета текущего поколения: делают свою уборку и + запускают reconnect-цикл **под текущим поколением**, если цикл под ним ещё не активен. + Для сокета старого поколения — только объявления (`OnDisconnect`, `OnSessionEnded`, + `_disconnectTcs`), без sweep'а, без смены состояния и без цикла: там уже есть владелец. +- `ReconnectLoopAsync(generation)`: каждая итерация под блокировкой проверяет владение; + retire предыдущей попытки — под блокировкой, без смены поколения. +- `OnConnectHandlerFailedAsync`: ту же уборку, что и сегодня, и цикл под поколением + сессии, чей handler упал, с seed'ом счётчика попыток. Если цикл под этим поколением уже + активен (handler упал у его же попытки), цикл просто продолжает — его счётчик и так растёт. +- `OnceOpen` для сессии не текущего поколения — отказ (сокет закроет тот, кто его создал, + см. 2.4). + +### 2.3. Окно 1: атомарное освобождение цикла + +`_reconnectLoopGeneration` заменяет `_reconnectLoop` + `IsCompleted`. Цикл, увидев после +попытки открытый сокет, освобождает метку **под `_transitionLock` и с повторной проверкой +`ShouldBeConnected()` под той же блокировкой**. `OnceClose` принимает решение «запускать ли +цикл» под той же блокировкой. Двух исходов достаточно, третьего нет: либо `OnceClose` видит +активный цикл (и тот, взяв блокировку, увидит уже закрытый сокет и продолжит крутиться), либо +цикл уже освободился (и `OnceClose` запускает новый). + +### 2.4. Окно 4 и «сокет, созданный во время гонки» + +`ConnectCoreAsync(generation, ct)` (бывший `ConnectInternalAsync`) проверяет владение под +блокировкой в трёх точках: после захвата `_connectLock` (до создания сокета), **вместе с +установкой `ws`** (создание сокета и установка сессии — одна критическая секция, так что +перекрывающий `TakeOver` либо застаёт `ws == null`, либо забирает уже созданный сокет) и после +`await socket.Connect()`. Потеряв владение после подключения, он закрывает свой сокет сам, +если тот всё ещё в `ws`, и выходит. + +`ChangeServer` проверяет владение после каждого `await` и после каждого consumer-callback'а +(`SetConnectionState`, sweep — continuations consumer'а выполняются inline, `NotifySessionEnded`). +Запись `url`/`config` — под блокировкой вместе с проверкой. Уведомление `Connecting` +переносится **после** захвата: handler, вызвавший из него `Disconnect()`, должен победить. + +### 2.5. Что видит вызывающий, когда его перекрыли + +| Операция | Перекрыта `Disconnect` | Перекрыта `ChangeServer`/`Connect` | +|---|---|---| +| `ChangeServer(A)` | `NotConnectedException` («Client has been disconnected») | `OperationCanceledException` с текстом, называющим перекрывшую операцию; проверка после `WaitForConnectionAsync` — по `url != A` | +| `Connect()` | `NotConnectedException` (как сейчас, из `WaitForConnectionAsync`) | успех, если клиент подключён — контракт `Connect` («OperationCanceledException только от токена вызывающего») сохраняется | +| `Disconnect()` | — | молча: не отправляет финальное `Disconnected`-уведомление, если поколение ушло | + +`ChangeServer`, перекрытый другим `ChangeServer`, уже разобрал старое соединение — это не +откатывается, перекрывший владеет остатком. + +### 2.6. Окно 2 и хвост 6: отправка под блокировкой, наблюдаемая отправка + +`Request`/`GRequest` берут сокет и запускают отправку **под `_transitionLock`**: +синхронный префикс `SendMessageAsync` (захват `_sendLock` сокета и выдача записи в +`ClientWebSocket`) выполняется до освобождения блокировки, так что retirement, идущий под той +же блокировкой, либо застаёт запрос ещё не отправленным (и `ws == null` его отклонит), либо +уже выданным в сокет. Остаток: отправка, вставшая в очередь за другой отправкой того же +сокета, повторно проверяет `_isIntentionalDisconnect` после `_sendLock`; между этой проверкой +и записью блокировки нет — это несколько инструкций, задокументировано. + +`WebSocketClient`: +- `public Task SendMessageAsync(byte[] message)` — возвращает `Task`; не-Open сокет → + faulted `InvalidOperationException`; `Connect()` из отправки удалён; исключения `SendAsync` + не глотаются; `_sendLock` сериализует целые сообщения (заодно закрывает перемешивание + кадров двух >1 МБ сообщений, которое `ManagedWebSocket` не запрещает). +- `public void SendMessage(string)` — прежний fire-and-forget контракт (ping, consumer'ы): + ошибку сообщает через `OnError`, как и раньше, соединение не трогает. + +`Request` ждёт `Task` отправки; провал → `requestManager.Reject(id, DisconnectedException)`, +после чего `await Promise` отдаёт отказ вызывающему. Ожидание отправки перед ожиданием +ответа ничего не стоит: ответ не может прийти раньше, чем запрос ушёл. + +### 2.7. Хвост 5 + +`NotConnectedException()` без аргумента получает сообщение по умолчанию: «The client is not +connected to a server…». `ImmediateFail` бросает с текстом, называющим политику. + +### 2.8. Найдено прогоном Blazor-клиента (WASM), исправлено здесь же + +- **Двойной отчёт по таймауту попытки.** В браузере отменённый `ConnectAsync` бросает + `WebSocketException` (ConnectFailure), а не `OperationCanceledException`, и общий `catch` + в `WebSocketClient.ConnectAsync` репортил его как вторую ошибку той же попытки. Теперь любое + исключение при `_cancellationToken.IsCancellationRequested || _isIntentionalDisconnect` — + это наша отмена, отчёт уже сделал тот, кто отменял. +- **Обрыв установленного соединения как «Initial connection failed».** Receive-loop вызывал и + `_onConnectionError` (написан для провала handshake), и `CallOnDisconnected`. Теперь + единственный репортёр — `OnceClose` (`ReportFailureAsCloseAsync`); в браузере + `WebSocketException` на открытом сокете классифицируется как network drop, пустое сообщение + заменяется кодом ошибки (`Describe`). Страховка в `OnConnectionFailed`: открытая сессия + (`IsOpened`) описывается как «Connection lost», а не «Initial connection failed». +- **Документация `UseCheckHealth`/`InactivityTimeout`** приведена к коду: детект тишины + требует `UseCustomPing`, и это намеренно (idle-соединение без подписок молчит по определению). +- Тест `TestFailureOfAnEstablishedConnectionIsReportedOnce` с `MalformedFrameServer` (кадр с + зарезервированным opcode) — воспроизводим в .NET; отмена handshake в .NET приходит как OCE и + юнит-тестом не покрывается, проверена в Blazor. + +## 3. Тесты + +Новые классы в `Tests/Xrpl.Tests/Client/` (MSTest, префикс `TestU`): + +- **`TestUConnectionTransitionOwner`**: + - `Disconnect()` из `OnSessionEnded`-handler'а внутри `ChangeServer` побеждает: клиент + остаётся `Disconnected`, `IsConnected()` false, `ChangeServer` бросает + `NotConnectedException`, второй сервер не получает handshake (окно 4, детерминированно). + - второй `ChangeServer(C)`, запущенный из `OnSessionEnded` первого `ChangeServer(B)`: + первый бросает `OperationCanceledException`, клиент оказывается на C, ровно один + `OnConnected`. + - `ChangeServer`, запущенный из `OnConnectionStatus(RestoringConnection)` быстрого + reconnect'а (сервер молчит на ping, `SilentOnPingServer`): fast reconnect отступает, + клиент подключается к новому серверу один раз, `RestoringConnection` после `Connected` + не приходит (окно 3 в его достижимой форме). + - сокет, закрывающийся сразу после успешной попытки reconnect-цикла (`CloseAfterHandshakeServer` + закрывает первые N соединений сразу после handshake): клиент всё равно доходит до сервера, + когда тот перестаёт закрывать (окно 1 в его достижимой форме; сам зазор в несколько + инструкций мок не открывает — это сказано в remarks теста). + - `Disconnect()` из `OnConnected`-handler'а: `OnceOpen` не сообщает `Connected` поверх + `Disconnected` и не запускает ping-таймер (найдено cold-review). + - `Connect()` после `Disconnect()` к серверу, который поднимется позже: клиент остаётся + reconnecting, а не «closed permanently» — `_isIntentionalDisconnect` следует за поколением + (найдено cold-review). + - `ImmediateFail` даёт непустое сообщение, содержащее «not connected»; + `new NotConnectedException()` тоже (в том же классе, не отдельным). +- **`TestWebSocketClient`** (дополнение): `SendMessage` на закрытом сокете не поднимает + `OnConnectionError` и не уничтожает сокет; `SendMessageAsync` на закрытом сокете — faulted. +- `TestUReconnectSessionRaces`, `TestURequestDuringServerSwitch`, `TestUFastReconnectSettling`, + `TestUChangeServerFailure`, `TestUSessionEndedNotification`, `TestUOnConnectedHandlerFailure` + остаются регрессионной сеткой; их remarks про `_reconnectLoop` обновляются. + +## 4. Что не меняется + +- Публичный API `Connection`: `ws`, `WebsocketSendAsync`, события. `WebsocketSendAsync` + остаётся для совместимости (тесты `TestSubscribe` его используют), `Request` им больше не + пользуется. +- Порядок «`ws` очищается до sweep'а» из #177 сохраняется: `TakeOver` забирает сокет первым. +- `_isIntentionalDisconnect`, per-socket трекинг `_userInitiatedSockets`, `_pingTimeoutSocket` + / `_networkDropSocket` — как есть; они про классификацию закрытия, не про владение. +- Семантика `OnSessionEnded`, `OnDisconnect`, `OnConnectionStatus` — как есть. + +## 5. Версия и changelog + +11.3.2.0 выпущена (тег v11.3.2). Это изменение меняет контракт (`ChangeServer` может бросить +«перекрыт», `WebSocketClient.SendMessage` больше не переподключает) — minor: **11.4.0.0**, +`PackageVersion` в `Xrpl/Xrpl.csproj`, раздел в `CHANGES.md` с датой релиза, проставляемой +при выпуске.