diff --git a/CHANGES.md b/CHANGES.md index 6153cefc..f8f9f229 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,30 @@ # Changes +## 11.5.0.0 13/09/2026 + +* **What happened to the connection is readable from the type, instead of the message text** (the follow-up to #179). 11.4.0 made the behaviour correct - one owner per transition, an operation that was overtaken says so - but gave the caller no way to read that answer. `NotConnectedException` carried five different events and `OperationCanceledException` two, so the only way to tell "the consumer disconnected the client" from "this endpoint is not answering" was to classify by message text - which the release notes of 11.3.2.0 told consumers not to do, while the library gave them no type capable of it. + * six new exception types, all deriving from the ones thrown today, so no `catch` clause changes meaning and no task changes status: `ClientDisconnectedException` and `ReconnectExhaustedException` (attempts spent, budget configured), `RequestRefusedException` for a request the caller asked not to have wait, `ConnectHandlerFailedException` (how many times the handler failed, and the handler's own exception), `ConnectionClosedPermanentlyException` for a node that closed with a code this client does not retry after, and `NotConnectingException` for a client with no attempt in progress. `ConnectionSupersededException` derives from `OperationCanceledException` and names the transition that took over and where it left the client + * **a broken `OnConnected` handler is no longer reported as a disconnect the consumer performed.** The give-up path ends by calling `Disconnect()` itself, so every point reading the permanently-disconnected flag answered "the client has been disconnected" - for a client that is down because its own handler is broken, where the node is answering and failing over would leave a healthy server. The cause now travels with the disconnect, written in the same critical section as the flag it qualifies. The two existing tests on that path made the defect plain: the same broken handler produced one type when it failed immediately and another when it failed a moment later + * **a request swept while the connection moved says which transition swept it, and where the client went.** This is the failure consumers meet most often, and it arrived as a bare `OperationCanceledException` reading "Connection was intentionally closed", indistinguishable from a cancellation of their own. Sweeps caused by the connection failing on its own - a network drop, a close being processed - deliberately keep the plain cancellation: that choice is what keeps an ordinary network drop out of consumers' critical logs, and the reason now reaches them on the status stream instead + * `ConnectionStatusInfo.StopReason` says why the client stopped, and only when it stopped: the first handshake failure against a server that is not up is announced and then retried, so it names no reason - whether one is named is decided by the same condition that decides whether the retry happens. A consumer reading any reason as terminal would otherwise fail over to another server while this one was still being dialled. + * `ConnectionStatusInfo.StopReason` says why the client stopped. `Disconnected` is announced from ten places and they differed only in text, so "still trying" against "gave up" was derivable only from the absence of `ReconnectInfo` - which is also what a client that never had a loop looks like. The reason goes on the notification rather than into `ReconnectInfo`, so `Reconnect != null` keeps its one meaning + * `WaitForConnectionOutcomeAsync` answers "did it come back?" with a value rather than an exception, on `Connection`, on `XrplClient` and on `IXrplClient` - where the wait was previously unreachable except through the connection object. `ConnectionWaitOutcome` names the case rather than folding "timed out", "gave up" and "nothing is running" into one `false`. `HasConnectionAsync` is untouched: adding a `CancellationToken` overload beside it would make argument-less calls ambiguous at the call site (CS0121) + * **the wait no longer polls.** It slept 100 ms at a time, which is slower than the event it waits for and, on a single-threaded host such as Blazor WebAssembly, more expensive than it looks - browser timers are throttled in a hidden tab, so a wait the event would satisfy at once stretched into seconds. It now sleeps on a signal completed by the one funnel every connection state passes through. Every check it made per pass is unchanged, so the answers are identical; the unit suite runs 43 s to 30 s + * `ChangeServer` reads the network id the way `Connect` does. `Connect` has carried that read across a teardown since 11.4.0, because a socket really does open for a moment before a failing handler brings it down; `ChangeServer` read it once, directly, so a connection that needed a second attempt failed the switch + * `ConnectionManager` is fixed rather than left alone. The readiness signal above is deliberately not built on it - it releases waiters when a connection is retired, and a retirement has to carry a waiting request over to the new connection rather than fail it - but it is public, reachable as `client.connection.connectionManager`, and notified from nine places on the connection's own threads. Nothing inside the SDK awaits it, so its defects had never shown: a waiter resumed **inside** `ResolveAllAwaiting`, which is called from inside `OnceOpen` before the `OnConnected` handler, and a registration landing during a notification either threw "Collection was modified" or was dropped and never resumed. The list is guarded, waiters are released outside the lock and resume asynchronously, completions are `TrySet*`, and a cancellation is `TrySetCanceled` rather than a faulted task + * **`StopAfterMaxAttempts` now actually stops the client.** Found while writing a test for one of the paths above, and present since before this change: a client that spent its reconnect budget announced `Disconnected` and then ran a second full series from attempt #1, announcing it again. The loop's exit clears the two fields that say a sequence is running for this generation, which is exactly what "none is running" looks like, so the close of the attempt that failed last was indistinguishable from the close that began the whole thing - and, with the cancellation source already released, started a fresh sequence with the counter at zero. The generation that gave up is now recorded and refused a new loop. Keyed by generation rather than flagged, so nothing has to reset it: generations only increase, and a `Connect()` or `ChangeServer` begins a new one - which is when asking again is the consumer's decision. A client that stopped on its own still answers the consumer asking, and that is asserted alongside + * **the wait and the status stream cannot disagree about how the connection ended.** Three ways they could, all found by the cold review of this change and all in the new code. The wait asked "is anything in progress?" before it asked "did something end?", and after a sequence ends there is no socket, no cancellation source and a `Disconnected` state - which is also exactly what a client nobody has called `Connect()` on looks like: a consumer who heard `ReconnectExhausted` on the status stream and confirmed it on the wait before failing over was told `NotConnecting`, and the outcome this whole change exists to deliver was reachable only by a caller who happened to be parked already. A close code the client does not reconnect after - 1002, 1003, 1007, 1010 - was announced as `ClosedPermanently` and left the wait nothing to recognise, so a parked caller was woken, found nothing, parked again, and was told a whole acquisition timeout later that the connection "was not established in time"; `ConnectionWaitOutcome.ClosedPermanently` and `ConnectionClosedPermanentlyException` are its counterpart. And the exhaustion the loop records, the permanent close, and the generation all used `0` for "none", which is the generation of a brand-new client - so the fields answered their own question with "yes" until the entry check above happened to mask it + * **one ownership check instead of three and a missing one.** A status announcement that speaks for a single transition of the connection is only true while that transition still owns it, and the check sat at the call sites: three had it, one did not, and the missing one was invisible for as long as the deduplication happened to swallow what it let through. Making the stop reason a change in its own right stopped it swallowing. The check now lives inside the one funnel every announcement passes through, in the same critical section that publishes the state - which also closes the window where a caller read "the sequence is still running", lost the processor, and published a `RestoringConnection` after the loop had ended the sequence and announced the ending + * a wait whose timeout is an exact multiple of the system clock tick - 15.625 ms, so one second and the default five minutes both are - spun on the connection's lock for the rest of the tick instead of timing out, because the deadline comparison was strict while the remaining time was already zero + * **every reason the client can stop for now has one outcome, under the same name, and one record behind both.** Each ending used to leave its own residue for a caller to recognise - a flag for the consumer's own disconnect, a generation for a spent reconnect budget - so an ending that left none was invisible to everything except the status stream, and a caller parked in the wait sat out its whole timeout to be told the connection "was not established in time" about a connection it had already been told was over. Two such endings were found, in three places. `ConnectionStopReason` is now recorded where the announcement is published, by the one method every status passes through, and the wait, its outcome value and the check every request makes all read that one record - so the three ways of asking cannot come to know different things. A request issued after the endpoint gave up used to answer "no connection attempt in progress. Call Connect() first", the one distinction the exception family exists to draw. The correspondence is asserted over the enums themselves rather than a list, which is how the last mismatch was found: `ConnectionStopReason.UserDisconnected` had been paired with a `ConnectionWaitOutcome.Disconnected`, and the outcome is renamed to match + * **every value of both enums is asserted by a test, and one value was deleted for failing to be.** Coverage was measured rather than assumed, and three holes came out of it: two endings whose exception was pinned but whose outcome value was not, and `ConnectionStopReason.InitialConnectionFailed`, which no test named because nothing can produce it. It is unreachable by construction - the branch that would name it is reached only for a socket that never opened as a session, and such a socket is always retried, which is the same fact `willReconnect` reads - and that was measured, not argued: the branch was instrumented and the whole suite run twice, once on the reason and once on `willReconnect` itself, for zero hits in 1328 tests. A value no consumer can observe invites a branch that never runs, so it is gone before release rather than kept as a defensive one; adding an enum member later is not a breaking change, and removing one is. The ending a consumer gets for a connection that never came up and stopped being retried is `ReconnectExhausted`, from the loop that stopped retrying it + * a runnable sample of the whole thing, `Tests/TestsClients/ConnectionLifecycleSample`. It needs no wallet and no funded account, because the subject is the connection: it produces each ending in turn - nothing in progress, an endpoint that is down, a request with no connection, a switch to a node that is down and the recovery from it, a consumer disconnect, a broken `OnConnected` handler, and an operation another one overtook - printing the status stream each one produced and then the type the caller got, with one line per ending saying what a consumer is supposed to do about it. `dotnet run --project Tests/TestsClients/ConnectionLifecycleSample` against the CI stand, or pass any node's URL + * **a client that has given up no longer calls itself connected.** The give-up path announces the ending, then rejects the requests in flight - which is what resumes the caller inside `Connect()` - and only then disconnects, so between the second step and the third the socket is still installed and still open. The wait began with a fast `IsConnected()` check and preferred "connected" to any ending, so a consumer was told the connection was up one instant after being told why it was over. The fast path is gone, the endings are read whether or not a socket happens to be open, and a recorded ending outranks a socket on its way out; an established connection clears the record, which is the counterpart. In the same window the cause of the ending was not yet recorded, so the answer named a consumer disconnect - the one thing that had not happened - and it is now recorded before the ending is announced rather than after. Found by CI: the whole suite passed on the development machine, where the socket usually closed in time + * **`Connect()` no longer reports success off a socket that is on its way out.** It opened with a fast path on `IsConnected()` alone, and in the window above the socket is still open - so a consumer reacting to a notification whose message ends "Call Connect() to retry" was told they were already connected, and nothing was started. The fast path now asks what the wait asks, under the same lock: an open socket, no permanent disconnect, and no recorded ending. Found by CodeRabbit on the commit that fixed the defect above + * **the ownership guard reached the four announcements that still lacked it.** A review found one - the reconnect loop's own `catch`, which checks ownership a line above the announcement rather than in the same critical section, so a takeover landing between the two overwrote the winner's state with a `RestoringConnection` from a loop that had already been superseded. Rather than fix the instance, every announcement in the file was audited: the fast reconnect's two, the handler-failure path that has not given up yet, and the one reported. Announcements that speak for the client as a whole rather than one transition - `Connecting` from the takeover that just took it, the `Disconnect()` paths, `Connected` from a connection that just opened - carry no generation by design + * **the ownership guard is now the compiler's job.** After the audit above, every status announcement carried its generation, so the parameter became required rather than optional. This is the only change of the set that closes the class instead of an instance: five reviews found five sites of it on this branch, one at a time. It can no longer be omitted - only passed wrongly, which is a smaller mistake and a visible one at the call site. A `Connected` that really happened is exempt from the spent-sequence seal, because suppressing it would leave the ending standing over a client whose socket is carrying traffic + * pinned by 46 tests, each asserting a type or a value and never a message. Four of them exist because they failed first: `Task.WhenAll` does **not** lose the subtype unless a faulted task is alongside it, a readiness signal armed only on takeover leaves a waiter spinning after a close that took over nothing, and a retry filter that cannot tell the client's own teardown from a peer operation reports a different failure depending on timing + ## 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. diff --git a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor index 808edad7..651f645b 100644 --- a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor +++ b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor @@ -547,8 +547,15 @@ CurrentConnectionState = statusInfo.ConnectionState; IsConnected = statusInfo.ConnectionState == XrpConnectionState.Connected; - AddStatusMessage($"[{statusInfo.ConnectionState}] {statusInfo.Message}", type); - Console.WriteLine($"Connection Status: [{statusInfo.ConnectionState}] {statusInfo.Message}"); + // The reason is shown only where it means something: on a notification that is not + // an ending it is None, and printing that on every line would bury the one place + // where "still trying" and "gave up" finally differ by more than their wording. + string stopped = statusInfo.StopReason == ConnectionStopReason.None + ? string.Empty + : $" [stopped: {statusInfo.StopReason}]"; + + AddStatusMessage($"[{statusInfo.ConnectionState}]{stopped} {statusInfo.Message}", type); + Console.WriteLine($"Connection Status: [{statusInfo.ConnectionState}]{stopped} {statusInfo.Message}"); StateHasChanged(); }); }; diff --git a/Tests/TestsClients/ConnectionLifecycleSample/ConnectionLifecycleSample.csproj b/Tests/TestsClients/ConnectionLifecycleSample/ConnectionLifecycleSample.csproj new file mode 100644 index 00000000..80e7ddf9 --- /dev/null +++ b/Tests/TestsClients/ConnectionLifecycleSample/ConnectionLifecycleSample.csproj @@ -0,0 +1,16 @@ + + + + Exe + net10.0 + enable + enable + latest + Xrpl.Samples.ConnectionLifecycle + + + + + + + diff --git a/Tests/TestsClients/ConnectionLifecycleSample/Program.cs b/Tests/TestsClients/ConnectionLifecycleSample/Program.cs new file mode 100644 index 00000000..7e5d91f7 --- /dev/null +++ b/Tests/TestsClients/ConnectionLifecycleSample/Program.cs @@ -0,0 +1,420 @@ +using System.Net; +using System.Net.Sockets; + +using Xrpl.Client; +using Xrpl.Client.Exceptions; + +namespace Xrpl.Samples.ConnectionLifecycle; + +/// +/// Every way an XrplClient connection can end, and what a consumer is supposed to do about each. +/// +/// +/// +/// Nothing here submits a transaction or needs a funded account: the subject is the connection +/// itself. Run it against any node - a local standalone rippled is the easiest: +/// +/// +/// docker compose -f .ci-config/docker-compose.ci.yml up -d +/// dotnet run --project Tests/TestsClients/ConnectionLifecycleSample +/// dotnet run --project Tests/TestsClients/ConnectionLifecycleSample -- wss://s.altnet.rippletest.net:51233 +/// +/// +/// The scenarios that need a server which is not answering use a port nothing is listening on, so +/// only the one node is required. +/// +/// +internal static class Program +{ + /// + /// The address the CI stand publishes on, and deliberately not localhost: that name + /// resolves to ::1 first on a dual-stack host while the container publishes on IPv4 + /// only, so every connection pays a failed IPv6 attempt first. Harmless with the default + /// attempt timeout and fatal with the short ones below, which is a trap worth not shipping in + /// a sample. + /// + private static string _server = "ws://127.0.0.1:6006"; + + private static async Task Main(string[] args) + { + if (args.Length > 0) + { + _server = args[0]; + } + + Console.WriteLine($"Node under test: {_server}"); + Console.WriteLine("Each scenario prints the status stream it produced, then the answer a caller gets."); + + try + { + await NothingIsInProgress(); + await TheEndpointIsNotAnswering(); + await AConnectionThatWorks(); + await ARequestWithNoConnection(); + await ASwitchToAnEndpointThatIsDown(); + await AConsumerDisconnect(); + await ABrokenConnectHandler(); + await AnOperationThatWasOvertaken(); + } + catch (Exception unexpected) + { + Console.WriteLine(); + Console.WriteLine($"The sample itself failed: {unexpected.GetType().Name}: {unexpected.Message}"); + return 1; + } + + Console.WriteLine(); + Console.WriteLine("Done. Every ending above is a distinct type and a distinct ConnectionStopReason,"); + Console.WriteLine("which is what lets a consumer react without reading message text."); + return 0; + } + + /// + /// A client nobody has called Connect() on. Not an error state - an actionable one. + /// + private static async Task NothingIsInProgress() + { + Scenario("A client that was never told to connect"); + + XrplClient client = new XrplClient(UnusedEndpoint()); + + ConnectionWaitOutcome outcome = + await client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(1)); + + Console.WriteLine($" outcome: {outcome}"); + Report(await Caught(() => client.connection.WaitForConnectionAsync(TimeSpan.FromSeconds(1)))); + } + + /// + /// The endpoint is not answering and the client has stopped trying. This is the one ending + /// that justifies failing over to another server. + /// + private static async Task TheEndpointIsNotAnswering() + { + Scenario("An endpoint that is down, with a reconnect budget that runs out"); + + // Without StopAfterMaxAttempts the loop keeps trying for ever and there is no ending to + // report - which is the right default for a long-lived client, and the wrong one for a + // client that is supposed to move to another node. + XrplClient client = new XrplClient(UnusedEndpoint(), new XrplClient.ClientOptions + { + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ReconnectBaseDelay = TimeSpan.FromMilliseconds(200), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(400), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + UseCustomPing = false, + }); + + Trace(client); + Report(await Caught(() => client.Connect())); + + // The same answer from a caller arriving afterwards, and from a request. All three ways of + // asking read one record, so a consumer cannot get "this endpoint gave up" from one and + // "you never connected" from another. + Console.WriteLine($" a later caller: {await client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(1))}"); + Report(await Caught(() => ServerInfo(client))); + + await client.Disconnect(); + } + + private static async Task AConnectionThatWorks() + { + Scenario("A connection that comes up"); + + XrplClient client = await Connected(); + + Console.WriteLine($" outcome: {await client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5))}"); + Console.WriteLine($" IsConnected: {client.connection.IsConnected()}"); + + await client.Disconnect(); + } + + /// + /// What a request does while there is no connection is a policy, not an accident. + /// + private static async Task ARequestWithNoConnection() + { + Scenario("A request issued while the client is not connected"); + + XrplClient refuses = new XrplClient(UnusedEndpoint(), new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + MaxReconnectAttempts = 1, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + UseCustomPing = false, + }); + + // Started and deliberately not awaited: the request has to be issued while an attempt is + // in progress, which is the case the policy is about. + Task connecting = Swallow(refuses.Connect()); + await Task.Delay(300); + + Console.WriteLine(" RequestFailurePolicy.ImmediateFail:"); + Report(await Caught(() => ServerInfo(refuses)), indent: " "); + + await refuses.Disconnect(); + await connecting; + + Console.WriteLine(" RequestFailurePolicy.WaitForConnection: the request waits for the connection instead,"); + Console.WriteLine(" and fails with the same typed ending if the connection never arrives."); + } + + /// + /// A switch to a node that is down leaves the client stopped - and still recoverable. + /// + private static async Task ASwitchToAnEndpointThatIsDown() + { + Scenario("Switching to an endpoint that is down, then coming back"); + + XrplClient client = await Connected(new XrplClient.ClientOptions + { + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ReconnectBaseDelay = TimeSpan.FromMilliseconds(200), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(400), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + UseCustomPing = false, + }); + + Trace(client); + Report(await Caught(() => client.connection.ChangeServer(UnusedEndpoint()))); + + // The client stays stopped until the consumer decides otherwise - that is what a terminal + // ending means - and deciding otherwise is one call. + Console.WriteLine(" recovering by switching back to a node that is up:"); + await client.connection.ChangeServer(_server); + Console.WriteLine($" IsConnected: {client.connection.IsConnected()}"); + + await client.Disconnect(); + } + + private static async Task AConsumerDisconnect() + { + Scenario("The consumer disconnects the client"); + + XrplClient client = await Connected(); + Trace(client); + + await client.Disconnect(); + + Console.WriteLine($" outcome: {await client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(2))}"); + Report(await Caught(() => ServerInfo(client))); + + // Nothing is going to bring this connection back on its own, which is exactly why the + // ending has a type of its own: failing over here would be leaving a healthy node. + Console.WriteLine(" Connect() is the way out, and it works:"); + await client.Connect(); + Console.WriteLine($" IsConnected: {client.connection.IsConnected()}"); + await client.Disconnect(); + } + + /// + /// The node is fine and this side is broken. Failing over would be leaving a healthy server. + /// + private static async Task ABrokenConnectHandler() + { + Scenario("An OnConnected handler that keeps throwing"); + + XrplClient client = new XrplClient(_server, new XrplClient.ClientOptions + { + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ReconnectBaseDelay = TimeSpan.FromMilliseconds(200), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(400), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + UseCustomPing = false, + }); + + client.connection.OnConnected += () => + throw new InvalidOperationException("restoring subscriptions failed"); + + Trace(client); + Report(await Caught(() => client.Connect())); + + await client.Disconnect(); + } + + /// + /// An operation another one overtook reports the winner rather than a bare cancellation. + /// + /// + /// The overtaking operation here is a second switch. A Disconnect() overtaking a switch + /// is the other half of the same rule and reports ClientDisconnectedException, because + /// the reaction a consumer owes it is the opposite one. + /// + private static async Task AnOperationThatWasOvertaken() + { + Scenario("A switch that another switch overtook"); + + XrplClient client = await Connected(new XrplClient.ClientOptions + { + MaxReconnectAttempts = 20, + ReconnectBaseDelay = TimeSpan.FromSeconds(2), + ReconnectMaxDelay = TimeSpan.FromSeconds(2), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + Trace(client); + + // Issued from the first switch's own status notification, which lands it inside that + // switch every time. A consumer meets this shape whenever a status handler reacts to the + // connection - "this node is not answering, go to the other one" is exactly such a + // handler. + bool overtaking = false; + client.connection.OnConnectionStatus += status => + { + if (status.ConnectionState == XrpConnectionState.RestoringConnection && !overtaking) + { + overtaking = true; + _ = Swallow(client.connection.ChangeServer(_server)); + } + }; + + Report(await Caught(() => client.connection.ChangeServer(UnusedEndpoint()))); + + Console.WriteLine($" the winner is where the client ended up - IsConnected: {client.connection.IsConnected()}"); + + // A Disconnect() that overtakes a switch reports ClientDisconnectedException instead: the + // client is down because it was asked to be, which calls for the opposite reaction, so the + // two cases do not share a type. + await client.Disconnect(); + } + + /// + /// The point of the whole exercise: one catch per ending, and a different reaction for + /// each. None of it reads message text. + /// + private static void Report(Exception? error, string indent = " ") + { + if (error is null) + { + Console.WriteLine($"{indent}completed without an exception"); + return; + } + + switch (error) + { + case ReconnectExhaustedException exhausted: + Console.WriteLine($"{indent}{nameof(ReconnectExhaustedException)} after {exhausted.Attempts} of {exhausted.MaxAttempts} attempts"); + Console.WriteLine($"{indent} -> this endpoint is not answering. Fail over to another server."); + break; + + case ConnectHandlerFailedException handler: + Console.WriteLine($"{indent}{nameof(ConnectHandlerFailedException)} after {handler.Failures} failure(s)"); + Console.WriteLine($"{indent} caused by: {handler.InnerException?.GetType().Name}: {handler.InnerException?.Message}"); + Console.WriteLine($"{indent} -> the node is fine and this side is broken. Fix the handler; do not fail over."); + break; + + case ConnectionClosedPermanentlyException: + Console.WriteLine($"{indent}{nameof(ConnectionClosedPermanentlyException)}"); + Console.WriteLine($"{indent} -> the node closed with a code that says retrying is pointless. Use another server."); + break; + + case ClientDisconnectedException: + Console.WriteLine($"{indent}{nameof(ClientDisconnectedException)}"); + Console.WriteLine($"{indent} -> the consumer took the client down. Only Connect() brings it back."); + break; + + case NotConnectingException: + Console.WriteLine($"{indent}{nameof(NotConnectingException)}"); + Console.WriteLine($"{indent} -> nothing is being attempted. Call Connect()."); + break; + + case RequestRefusedException: + Console.WriteLine($"{indent}{nameof(RequestRefusedException)}"); + Console.WriteLine($"{indent} -> the connection was being rebuilt and this caller asked not to wait. Retry once connected."); + break; + + case ConnectionSupersededException superseded: + Console.WriteLine($"{indent}{nameof(ConnectionSupersededException)}: overtaken by {superseded.Kind}"); + Console.WriteLine($"{indent} -> a later operation owns the connection. Its result is the one that counts."); + break; + + case NotConnectedException other: + Console.WriteLine($"{indent}{other.GetType().Name}: {other.Message}"); + Console.WriteLine($"{indent} -> the base type still catches every one of the above."); + break; + + case System.TimeoutException: + Console.WriteLine($"{indent}{nameof(System.TimeoutException)}"); + Console.WriteLine($"{indent} -> it did not come up in the time allowed. Nothing has given up; waiting longer may still work."); + break; + + default: + Console.WriteLine($"{indent}{error.GetType().Name}: {error.Message}"); + break; + } + } + + /// Prints the status stream, which carries the same endings as a value. + private static void Trace(XrplClient client) + { + client.connection.OnConnectionStatus += status => + { + string stopped = status.StopReason == ConnectionStopReason.None + ? string.Empty + : $" [stopped: {status.StopReason}]"; + + Console.WriteLine($" status: {status.ConnectionState}{stopped} - {status.Message}"); + }; + } + + /// The cheapest real request there is - it needs no account and no funds. + private static Task ServerInfo(XrplClient client) => + client.connection.Request(new Dictionary { { "command", "server_info" } }); + + private static async Task Connected(XrplClient.ClientOptions? options = null) + { + XrplClient client = new XrplClient( + _server, + options ?? new XrplClient.ClientOptions { UseCustomPing = false }); + + await client.Connect(); + return client; + } + + private static async Task Caught(Func operation) + { + try + { + await operation(); + return null; + } + catch (Exception error) + { + return error; + } + } + + private static async Task Swallow(Task operation) + { + try + { + await operation; + } + catch + { + // The scenario reports through its own caller; this one exists only to be started. + } + } + + /// A loopback port nothing is listening on, so "the server is down" needs no server. + private static string UnusedEndpoint() + { + TcpListener listener = new TcpListener(IPAddress.Loopback, port: 0); + listener.Start(); + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + + return $"ws://127.0.0.1:{port}"; + } + + private static void Scenario(string title) + { + Console.WriteLine(); + Console.WriteLine($"== {title}"); + } +} diff --git a/Tests/TestsClients/ConnectionLifecycleSample/README.md b/Tests/TestsClients/ConnectionLifecycleSample/README.md new file mode 100644 index 00000000..2ec33a1d --- /dev/null +++ b/Tests/TestsClients/ConnectionLifecycleSample/README.md @@ -0,0 +1,84 @@ +# Connection lifecycle sample + +Every way an `XrplClient` connection can end, and what a consumer is supposed to do about each. + +Nothing here submits a transaction or needs a funded account — the subject is the connection +itself. The sample runs eight scenarios in order, prints the status stream each one produced, and +then prints the type the caller got and the one sentence that says how to react to it. + +## Run it + +One node is all it needs. The scenarios that require a server which is **not** answering bind a +loopback port and release it, so they need nothing running. + +Against the CI stand (the default, `ws://127.0.0.1:6006`): + +```bash +docker compose -f .ci-config/docker-compose.ci.yml up -d +``` + +```bash +dotnet run --project Tests/TestsClients/ConnectionLifecycleSample +``` + +Against any other node — pass its URL: + +```bash +dotnet run --project Tests/TestsClients/ConnectionLifecycleSample -- wss://s.altnet.rippletest.net:51233 +``` + +The whole run takes under a minute: most of it is the reconnect backoff of the scenarios that are +supposed to fail. + +Prefer `127.0.0.1` over `localhost` for a local node. On a dual-stack host `localhost` resolves to +`::1` first while Docker publishes on IPv4 only, so every connection pays a failed IPv6 attempt — +harmless with the default attempt timeout, fatal with the short ones this sample uses to stay +quick. + +## Reading the output + +Two kinds of line. Indented four spaces is the status stream, exactly as `OnConnectionStatus` +delivers it: + +``` + status: RestoringConnection - Reconnecting in 0.4 seconds... (attempt #2) + status: Disconnected [stopped: ReconnectExhausted] - Reconnection stopped after 2 attempts. +``` + +`[stopped: ...]` appears only when the client has stopped. A `Disconnected` without it is a +failure the client is about to retry, which is why the first handshake failure against a server +that is down names no reason: naming one would tell a consumer to fail over while this node is +still being dialled. + +Indented two spaces is what the caller got: + +``` + ReconnectExhaustedException after 2 of 2 attempts + -> this endpoint is not answering. Fail over to another server. +``` + +## The scenarios + +| Scenario | Ending | What it is for | +|---|---|---| +| A client that was never told to connect | `NotConnectingException` | "Nothing is in progress" is an answer, not an error | +| An endpoint that is down | `ReconnectExhaustedException` | The one ending that justifies failing over. Needs `StopAfterMaxAttempts` | +| A connection that comes up | `ConnectionWaitOutcome.Connected` | The value-returning wait, with no `catch` | +| A request with no connection | `RequestRefusedException` | `RequestFailurePolicy` is a decision, not an accident | +| A switch to a node that is down | `ReconnectExhaustedException`, then recovery | A stopped client stays stopped, and one call brings it back | +| The consumer disconnects | `ClientDisconnectedException` | Never fail over here — the client was asked to be down | +| A broken `OnConnected` handler | `ConnectHandlerFailedException` | The node is fine and this side is broken. The handler's own exception is the `InnerException` | +| A switch another switch overtook | `ConnectionSupersededException` | The overtaken operation names the winner instead of a bare cancellation. A `Disconnect()` overtaking a switch is the other half of the rule and reports `ClientDisconnectedException` — the opposite reaction, so not the same type | + +## What to take from it + +`Report` in `Program.cs` is the part worth copying: one `catch` per ending, each with a different +reaction, and not one line of it reads message text. That is the whole point of the typed +connection outcomes — before them all of these arrived as the same `NotConnectedException`, and +telling them apart meant matching on strings that were free to change. + +The same endings are readable three ways, and all three agree: + +- as an exception from `WaitForConnectionAsync`, `Connect`, `ChangeServer` or a request; +- as a value from `WaitForConnectionOutcomeAsync`; +- as `ConnectionStatusInfo.StopReason` on the status stream. diff --git a/Tests/Xrpl.Tests/Client/ClosesWithCodeServer.cs b/Tests/Xrpl.Tests/Client/ClosesWithCodeServer.cs new file mode 100644 index 00000000..2113538b --- /dev/null +++ b/Tests/Xrpl.Tests/Client/ClosesWithCodeServer.cs @@ -0,0 +1,78 @@ +using System.Net.Sockets; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; + +namespace Xrpl.Tests +{ + /// + /// WebSocket server that answers requests normally until told to stop, then closes the + /// connection with a fixed status code. + /// + /// + /// The close codes the client does not reconnect after - 1002, 1003, 1007, 1010 - are the only + /// way a connection ends with neither a consumer Disconnect() nor a reconnect sequence + /// behind it, and no other test server can produce one: the shared mock never closes, and + /// CloseAfterHandshakeServer sends a close frame carrying no code at all, which the + /// client reads as "reconnect". + /// + internal sealed class ClosesWithCodeServer : 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 _closeCode; + private readonly SemaphoreSlim _closeNow = new SemaphoreSlim(initialCount: 0); + + public ClosesWithCodeServer(int closeCode) + { + _closeCode = closeCode; + StartAccepting(); + } + + protected override bool ServesManyClients => true; + + /// Releases the serving loop to send the close frame. + public void CloseNow() => _closeNow.Release(); + + protected override async Task ServeAsync(NetworkStream stream) + { + Task closeRequested = _closeNow.WaitAsync(Token); + + while (!Token.IsCancellationRequested) + { + Task nextRequest = ReadTextFrameAsync(stream); + Task first = await Task.WhenAny(nextRequest, closeRequested).ConfigureAwait(false); + + if (first == closeRequested) + { + // FIN + close opcode, two payload bytes, the code big-endian. + byte[] close = + { + 0x88, 0x02, (byte)(_closeCode >> 8), (byte)(_closeCode & 0xFF), + }; + + await stream.WriteAsync(close, Token).ConfigureAwait(false); + await stream.FlushAsync(Token).ConfigureAwait(false); + return; + } + + string? request = await nextRequest.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/DropsFirstServerInfoServer.cs b/Tests/Xrpl.Tests/Client/DropsFirstServerInfoServer.cs new file mode 100644 index 00000000..bba59272 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/DropsFirstServerInfoServer.cs @@ -0,0 +1,74 @@ +using System.Net.Sockets; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; + +namespace Xrpl.Tests +{ + /// + /// WebSocket server that drops the connection the first time it is asked for + /// server_info, and behaves normally on every connection after that. + /// + /// + /// + /// The handshake succeeds, so a client reaches the point where it believes it is connected and + /// sends its first request - and only then does the connection go away. That is the window in + /// which reading the network id happens: a caller that reads it once, directly, fails the whole + /// operation, while one that carries the read across a teardown recovers and completes. + /// + /// + /// The shared mock cannot produce this: it answers every request on the connection it accepted, + /// so the teardown never lands between "connected" and "first answer". Dropping the first + /// request only, rather than every one, is what makes the recovery observable instead of just + /// the failure. + /// + /// + internal sealed class DropsFirstServerInfoServer : 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 int _dropsLeft = 1; + + public DropsFirstServerInfoServer() + { + StartAccepting(); + } + + /// The client reconnects after the drop, so the next connection has to be served. + protected override bool ServesManyClients => true; + + protected override async Task ServeAsync(NetworkStream stream) + { + while (!Token.IsCancellationRequested) + { + string request = await ReadTextFrameAsync(stream).ConfigureAwait(false); + if (request == null) + { + return; + } + + using JsonDocument document = JsonDocument.Parse(request); + string command = document.RootElement.TryGetProperty("command", out JsonElement value) + ? value.GetString() + : null; + + if (command == "server_info" && Interlocked.Decrement(ref _dropsLeft) >= 0) + { + // Returning closes the socket without answering: the request the client is + // waiting for dies with the connection. + return; + } + + 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/Exceptions/TestUConnectionOutcomeTypes.cs b/Tests/Xrpl.Tests/Client/Exceptions/TestUConnectionOutcomeTypes.cs new file mode 100644 index 00000000..b64ff2f0 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/Exceptions/TestUConnectionOutcomeTypes.cs @@ -0,0 +1,242 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +using Xrpl.Client; +using Xrpl.Client.Exceptions; + +namespace Xrpl.Tests.Client.Exceptions +{ + /// + /// The types that say what happened to the connection, rather than leaving it in the message + /// text - see specs/2026-09-09-connection-outcome-api.md. + /// + /// + /// + /// carried five different events and + /// two, so a consumer that had to react differently to + /// each was left classifying by text - which the release notes of 11.3.2.0 told them not to do, + /// while the library gave them no type capable of it. + /// + /// + /// These tests are about the types themselves: what they derive from, and what they carry. + /// Which path produces which type is a question about the connection, and lives in + /// TestUConnectionOutcomes. + /// + /// + [TestClass] + public class TestUConnectionOutcomeTypes + { + /// + /// The failure that caused a refusal survives on the exception that reports it. + /// + /// + /// had a single message constructor, so a subtype that + /// has a cause - the handler exception behind + /// - had nowhere to put it: there is no setter + /// for . The whole taxonomy rests on this one + /// constructor existing. + /// + [TestMethod] + public void TestUNotConnectedExceptionKeepsTheFailureThatCausedIt() + { + InvalidOperationException cause = new InvalidOperationException("the handler threw"); + + NotConnectedException error = new NotConnectedException("gave up connecting", cause); + + Assert.AreSame(cause, error.InnerException); + Assert.AreEqual("gave up connecting", error.Message); + } + + /// + /// Every new type is still a . + /// + /// + /// This is what makes the change additive rather than breaking: the existing + /// catch (NotConnectedException) in consumer code, and the ones inside this library, + /// keep catching all five events. Only code that wants to tell them apart has to look. + /// + [TestMethod] + public void TestUEveryOutcomeIsStillANotConnectedException() + { + Assert.IsInstanceOfType(new ClientDisconnectedException("x")); + Assert.IsInstanceOfType(new ReconnectExhaustedException("x", attempts: 1, maxAttempts: 1)); + Assert.IsInstanceOfType(new RequestRefusedException("x")); + Assert.IsInstanceOfType(new ConnectHandlerFailedException("x", failures: 1)); + Assert.IsInstanceOfType(new NotConnectingException("x")); + } + + /// + /// Exhaustion says how many attempts were spent and what the budget was. + /// + /// + /// Both numbers are reported, not just the limit, because a consumer deciding whether to + /// fail over wants to know the budget it actually configured was really spent. The raw + /// counter in the connection stands at MaxAttempts + 1 when the loop stops - it is + /// incremented at the head of a pass and the loop breaks on the pass that exceeds the + /// budget - so "6 of 5" is what a naive reading would publish. The throw site passes the + /// number of attempts made. + /// + [TestMethod] + public void TestUExhaustionCarriesTheAttemptsItSpent() + { + ReconnectExhaustedException error = new ReconnectExhaustedException( + "Connection failed permanently after 5 attempts. Reconnection has been stopped.", + attempts: 5, + maxAttempts: 5); + + Assert.AreEqual(5, error.Attempts); + Assert.AreEqual(5, error.MaxAttempts); + } + + /// + /// A failing OnConnected handler reports how often it failed, and with what. + /// + /// + /// This is the one outcome where the node is answering and the fault is on this side, so a + /// consumer that reacts to it by failing over to another server would be moving away from + /// a healthy node. The handler's own exception is what says why, and it used to reach the + /// caller only as text inside the message. + /// + [TestMethod] + public void TestUAFailingConnectHandlerCarriesItsCountAndItsCause() + { + InvalidOperationException cause = new InvalidOperationException("subscription refused"); + + ConnectHandlerFailedException error = new ConnectHandlerFailedException( + "Gave up connecting: the OnConnected handler failed 3 time(s) in a row.", + failures: 3, + innerException: cause); + + Assert.AreEqual(3, error.Failures); + Assert.AreSame(cause, error.InnerException); + } + + /// + /// Being overtaken is still a cancellation, and now says by what and where it went. + /// + /// + /// The four supersession sites already threw , so + /// deriving from it keeps every catch and every task status exactly as they were. + /// What the caller could not do before is tell "another operation took the connection over" + /// apart from "my own token was cancelled". + /// + [TestMethod] + public void TestUSupersessionIsACancellationThatNamesItsWinner() + { + ConnectionSupersededException error = new ConnectionSupersededException( + "Superseded by a later ChangeServer to wss://example.test:6006.", + ConnectionTransitionKind.ChangeServer, + supersededBy: "wss://example.test:6006"); + + Assert.IsInstanceOfType(error, "catch (OperationCanceledException) must keep catching this."); + Assert.AreEqual(ConnectionTransitionKind.ChangeServer, error.Kind); + Assert.AreEqual("wss://example.test:6006", error.SupersededBy); + } + + /// + /// It carries no cancellation token, because nobody cancelled anything. + /// + /// + /// A caller that filters its own cancellations with + /// catch (OperationCanceledException ex) when (ex.CancellationToken == myToken) + /// must not have that filter match here: the operation was overtaken by another operation, + /// not cancelled by the caller. This is the reason the constructor does not take a token. + /// + [TestMethod] + public void TestUSupersessionCarriesNobodysToken() + { + using CancellationTokenSource callerToken = new CancellationTokenSource(); + + ConnectionSupersededException error = new ConnectionSupersededException( + "Superseded by a later Connect().", + ConnectionTransitionKind.Connect); + + Assert.AreNotEqual(callerToken.Token, error.CancellationToken); + Assert.AreEqual(CancellationToken.None, error.CancellationToken); + Assert.IsNull(error.SupersededBy, "A Connect() that won left the client where it already was."); + } + + /// + /// Awaited directly, the subtype reaches the caller - and the task reports itself cancelled. + /// + /// + /// AsyncTaskMethodBuilder turns an escaping + /// into a cancelled task whatever its token says, so this is the behaviour the supersession + /// sites already had; the test pins that inheritance did not change it. + /// + [TestMethod] + public async Task TestUSupersessionSurvivesADirectAwait() + { + Task direct = SupersededAsync(); + + ConnectionSupersededException caught = + await Assert.ThrowsExactlyAsync(async () => await direct); + + Assert.AreEqual(ConnectionTransitionKind.Reconnect, caught.Kind); + Assert.AreEqual( + TaskStatus.Canceled, + direct.Status, + "An OperationCanceledException leaving an async method cancels its task."); + } + + /// + /// Through Task.WhenAll the subtype survives too, as long as nothing faulted. + /// + /// + /// Measured rather than assumed, and the measurement contradicted the expectation this test + /// was written from: WhenAll with cancellations and no faults stores the first + /// cancellation exception and rethrows that very instance, so + /// is still readable. Documenting the + /// pessimistic rule would have sent consumers looking for a workaround they do not need. + /// + [TestMethod] + public async Task TestUSupersessionSurvivesWhenAllWithNoFaults() + { + Task combined = Task.WhenAll(SupersededAsync(), Task.CompletedTask); + + ConnectionSupersededException caught = + await Assert.ThrowsExactlyAsync(async () => await combined); + + Assert.AreEqual(ConnectionTransitionKind.Reconnect, caught.Kind); + } + + /// + /// A fault alongside it loses the supersession entirely. + /// + /// + /// WhenAll prefers faults to cancellations: with any faulted task the combined task + /// faults, and - measured here rather than assumed - the cancellation is not recorded at + /// all, so it is absent from Task.Exception.InnerExceptions as well as from the + /// await. This is the one case where a caller cannot learn it was overtaken, and it + /// is why the XML documentation of the type says to await the operation itself. + /// + [TestMethod] + public async Task TestUAFaultAlongsideItLosesTheSupersession() + { + static async Task FaultedAsync() + { + await Task.Yield(); + throw new InvalidOperationException("something else went wrong"); + } + + Task combined = Task.WhenAll(SupersededAsync(), FaultedAsync()); + + await Assert.ThrowsExactlyAsync(async () => await combined); + + Assert.AreEqual(TaskStatus.Faulted, combined.Status); + Assert.IsFalse( + combined.Exception!.InnerExceptions.Any(e => e is ConnectionSupersededException), + "WhenAll records faults only: a cancellation alongside a fault is dropped, not merely hidden."); + } + + private static async Task SupersededAsync() + { + await Task.Yield(); + throw new ConnectionSupersededException("overtaken", ConnectionTransitionKind.Reconnect); + } + } +} diff --git a/Tests/Xrpl.Tests/Client/SilentOnPingAndLedgerServer.cs b/Tests/Xrpl.Tests/Client/SilentOnPingAndLedgerServer.cs new file mode 100644 index 00000000..cad1cdb7 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/SilentOnPingAndLedgerServer.cs @@ -0,0 +1,63 @@ +using System.Net.Sockets; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; + +namespace Xrpl.Tests +{ + /// + /// WebSocket server that answers neither ping nor ledger, and answers everything + /// else with the same server_info body. + /// + /// + /// Two silences, for two halves of one scenario. Not answering ping is what drives the + /// health check to declare the connection dead and hand it to the fast-reconnect path - the + /// shared mock answers pings itself, so its activity clock never runs out. Not answering + /// ledger is what keeps a request in flight while that happens, so the sweep the + /// reconnect performs has something to sweep. Answering everything else is what keeps the + /// connection up long enough for either to matter. + /// + internal sealed class SilentOnPingAndLedgerServer : WebSocketTestServerBase + { + private const string ServerInfoEnvelope = + "{\"id\":__ID__,\"status\":\"success\",\"type\":\"response\",\"result\":{\"info\":" + + "{\"build_version\":\"test-mock\",\"complete_ledgers\":\"1-1\",\"server_state\":\"full\"}}}"; + + public SilentOnPingAndLedgerServer() + { + StartAccepting(); + } + + /// The client reconnects when the health check gives up on the silence. + protected override bool ServesManyClients => true; + + protected override async Task ServeAsync(NetworkStream stream) + { + while (!Token.IsCancellationRequested) + { + string request = await ReadTextFrameAsync(stream).ConfigureAwait(false); + if (request == null) + { + return; + } + + using JsonDocument document = JsonDocument.Parse(request); + string command = document.RootElement.TryGetProperty("command", out JsonElement value) + ? value.GetString() + : null; + + if (command == "ping" || command == "ledger") + { + continue; + } + + 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/TestUConnectionManagerWaiters.cs b/Tests/Xrpl.Tests/Client/TestUConnectionManagerWaiters.cs new file mode 100644 index 00000000..4ceb21e8 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUConnectionManagerWaiters.cs @@ -0,0 +1,145 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +using Xrpl.Client; + +namespace Xrpl.Tests +{ + /// + /// as something a consumer can actually call. + /// + /// + /// + /// It is reachable from outside the library - client.connection.connectionManager is a + /// public field on a public type - and the connection notifies it from nine places, on its own + /// threads. Nothing inside the SDK awaits it, so its defects never showed; that makes them + /// latent, not absent, and the first consumer to call AwaitConnection() would meet all + /// of them at once. + /// + /// + /// These are unit tests of the class alone: no socket, no client. What they pin is the + /// behaviour any waiter is entitled to - resume off the notifying thread, survive a concurrent + /// notification, and be cancelled rather than faulted. + /// + /// + [TestClass] + public class TestUConnectionManagerWaiters + { + /// + /// A waiter resumes after the notification returns, not inside it. + /// + /// + /// The completion sources were created without + /// , so every parked waiter + /// resumed synchronously inside - which + /// the connection calls from inside OnceOpen, before the OnConnected handler + /// and while it holds the connection together. Consumer code running there is the defect + /// issue #177 was about, in a new place. + /// + [TestMethod] + public async Task TestUAWaiterDoesNotResumeInsideTheNotification() + { + ConnectionManager manager = new ConnectionManager(); + + int resumed = 0; + Task waiter = ResumeMarker(); + + async Task ResumeMarker() + { + await manager.AwaitConnection(); + Volatile.Write(ref resumed, 1); + } + + manager.ResolveAllAwaiting(); + int resumedInsideTheCall = Volatile.Read(ref resumed); + + await waiter; + + Assert.AreEqual( + 0, + resumedInsideTheCall, + "The waiter resumed inside ResolveAllAwaiting - which the connection calls from inside OnceOpen."); + Assert.AreEqual(1, Volatile.Read(ref resumed), "It still has to resume, just not there."); + } + + /// + /// A cancelled wait is cancelled, not faulted. + /// + /// + /// SetException(new OperationCanceledException(...)) produces a faulted task: the + /// rule that turns a cancellation into a cancelled task belongs to the async method + /// builder, not to . The difference is visible + /// to anyone reading Task.Status, combining the task with others, or leaving it + /// unobserved. + /// + [TestMethod] + public async Task TestUACancelledWaitIsCancelledRatherThanFaulted() + { + ConnectionManager manager = new ConnectionManager(); + + Task waiter = manager.AwaitConnection(); + manager.RejectAllAwaitingWithCancellation(); + + await Assert.ThrowsAsync(async () => await waiter); + Assert.AreEqual(TaskStatus.Canceled, waiter.Status); + } + + /// + /// Registering while a notification is going out does not corrupt the list or lose a waiter. + /// + /// + /// The waiters lived in a plain List<T> mutated without synchronisation, while + /// the connection notifies from its socket, reconnect and caller threads. Both failures are + /// real: an InvalidOperationException from a list modified while being iterated, and + /// a waiter added in that window and dropped by the reassignment that follows - which is the + /// worse of the two, because it never comes back and never says anything. + /// + [TestMethod] + public async Task TestURegisteringDuringANotificationLosesNobody() + { + for (int round = 0; round < 200; round++) + { + ConnectionManager manager = new ConnectionManager(); + List waiters = new List(); + + Task registering = Task.Run(() => + { + for (int i = 0; i < 20; i++) + { + lock (waiters) + { + waiters.Add(manager.AwaitConnection()); + } + } + }); + + Task notifying = Task.Run(() => + { + for (int i = 0; i < 20; i++) + { + manager.ResolveAllAwaiting(); + } + }); + + await Task.WhenAll(registering, notifying); + + // Whatever the interleaving was, everyone still parked is released by this one. + manager.ResolveAllAwaiting(); + + Task all; + lock (waiters) + { + all = Task.WhenAll(waiters); + } + + Task finished = await Task.WhenAny(all, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.AreSame(all, finished, $"A waiter was dropped and never resumed (round {round})."); + await all; + } + } + } +} diff --git a/Tests/Xrpl.Tests/Client/TestUConnectionOutcomes.cs b/Tests/Xrpl.Tests/Client/TestUConnectionOutcomes.cs new file mode 100644 index 00000000..65440a91 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUConnectionOutcomes.cs @@ -0,0 +1,2112 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; + +using Xrpl.Client; +using Xrpl.Client.Exceptions; + +namespace Xrpl.Tests +{ + /// + /// Which path produces which outcome - see specs/2026-09-09-connection-outcome-api.md. + /// + /// + /// + /// The types themselves are covered in TestUConnectionOutcomeTypes. What is pinned here + /// is the mapping: an event of the connection, and the type a caller gets for it. Every + /// assertion is on a type or on a value, never on message text - which is the entire point of + /// the change. + /// + /// + /// Each test also asserts that the base type still catches, because the promise made to + /// consumers is that this is additive: a catch (NotConnectedException) written against + /// 11.4.0 keeps working. + /// + /// + [TestClass] + public class TestUConnectionOutcomes + { + private XrplClient _client; + + 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 CreateMockRippled StartMock(int port) + { + CreateMockRippled mock = new CreateMockRippled(port) { suppressOutput = true }; + mock.AddResponse("server_info", ServerInfoResponse()); + + Thread listenerThread = new Thread(() => mock.Start()) { IsBackground = true }; + listenerThread.Start(); + return mock; + } + + [TestCleanup] + public async Task MyTestCleanup() + { + if (_client != null) + { + await _client.Disconnect(); + _client = null; + } + } + + /// + /// A client that was never told to connect says so, rather than saying it is disconnected. + /// + /// + /// "Nothing is in progress" is a distinct, actionable answer - the caller has to call + /// Connect() - and it is neither "the consumer took the client down" nor "the + /// reconnect loop gave up". It used to arrive as a bare + /// alongside both of those. + /// + [TestMethod] + public async Task TestUAClientThatNeverConnectedReportsThatNothingIsInProgress() + { + int port = TestUtils.GetFreePort(); // nothing is listening, and nothing will be asked to + _client = new XrplClient($"ws://127.0.0.1:{port}"); + + NotConnectingException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.WaitForConnectionAsync(TimeSpan.FromSeconds(1))); + + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + } + + /// + /// A caller already waiting when the reconnect loop spends its budget is told the budget is + /// spent, and how big it was. + /// + /// + /// + /// This is the outcome that justifies a failover, and the one it was impossible to + /// recognise: it arrived as the same as a consumer's + /// own Disconnect(), which calls for the opposite reaction. + /// + /// + /// The wait has to begin before the loop gives up. Once the loop has released its + /// cancellation source and reported Disconnected, a fresh call finds no attempt in + /// progress at all and is answered by - a different, + /// equally correct answer to a different question. That is why the waiter here is + /// ChangeServer's own: it starts the attempt and then waits for it, so it is + /// already parked when the loop it left behind runs out of budget. Stopping a server and + /// racing to park a waiter before the loop finishes would test the same thing by timing. + /// + /// + [TestMethod] + public async Task TestUAWaiterLearnsTheReconnectBudgetWasSpent() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the mock."); + + int deadPort = TestUtils.GetFreePort(); // nothing is listening there, and never will be + + ReconnectExhaustedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{deadPort}")); + + Assert.AreEqual(2, error.MaxAttempts, "The budget the client was configured with."); + Assert.AreEqual( + error.MaxAttempts, + error.Attempts, + "Attempts made, not the raw counter - which stands one past the budget when the loop stops."); + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + } + finally + { + mock.Stop(); + } + } + + /// + /// Giving up on a broken OnConnected handler says the handler broke - not that the + /// consumer disconnected the client. + /// + /// + /// + /// The give-up path ends by calling Disconnect() itself, which sets the same + /// permanently-disconnected flag a consumer's own Disconnect() sets. Every point + /// that reads that flag then answered "the client has been disconnected" - the one thing + /// this event is not. The node is answering; what failed is code on this side, so a + /// consumer reacting by failing over would be leaving a healthy server. + /// + /// + /// The distinction was invisible and timing-dependent: the same broken handler produced one + /// type when it failed immediately and another when it failed a moment later, because only + /// the second left a request in flight to be rejected with the real reason. + /// + /// + [TestMethod] + public async Task TestUGivingUpOnABrokenConnectHandlerSaysTheHandlerBroke() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + InvalidOperationException thrownByHandler = new InvalidOperationException("handler is permanently broken"); + _client.connection.OnConnected += () => throw thrownByHandler; + + ConnectHandlerFailedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.Connect()); + + Assert.IsGreaterThanOrEqualTo(1, error.Failures, "The handler failed at least once before the client gave up."); + Assert.AreSame(thrownByHandler, error.InnerException, "The handler's own failure is what says why."); + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + + // The value-returning way of asking has to name the same case. It is a separate + // path - the exception is caught and translated - so a mapping that is wrong here + // is wrong for every consumer who prefers a value to a catch. + ConnectionWaitOutcome outcome = + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5)); + + Assert.AreEqual( + ConnectionWaitOutcome.ConnectHandlerFailed, + outcome, + "The outcome and the exception are two ways of asking one question."); + } + finally + { + mock.Stop(); + } + } + + /// + /// An operation that was overtaken says what overtook it and where the client ended up. + /// + /// + /// + /// The second ChangeServer is issued from the first one's own session-ended handler, + /// which lands it inside the first one's yields every time - the shape a consumer actually + /// hits, and deterministic where a race would not be. + /// + /// + /// Before this, the overtaken call got a bare , + /// indistinguishable from the caller's own token being cancelled. A consumer that showed + /// the user "could not connect, pick another node" for it was reporting a failure on a + /// client that was at that moment connecting normally somewhere else. + /// + /// + [TestMethod] + public async Task TestUAnOvertakenSwitchNamesTheSwitchThatWon() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + int thirdPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + CreateMockRippled third = StartMock(thirdPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + string thirdUrl = $"ws://127.0.0.1:{thirdPort}"; + int nested = 0; + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && Interlocked.Exchange(ref nested, 1) == 0) + { + await _client.connection.ChangeServer(thirdUrl); + } + }; + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{secondPort}")); + + Assert.AreEqual(ConnectionTransitionKind.ChangeServer, error.Kind); + Assert.AreEqual(thirdUrl, error.SupersededBy, "The caller is told where the client actually is."); + Assert.IsInstanceOfType( + error, + "catch (OperationCanceledException) must keep catching this."); + } + finally + { + first.Stop(); + second.Stop(); + third.Stop(); + } + } + + /// + /// A request refused because the caller asked not to wait says that, and not that the + /// endpoint is dead. + /// + /// + /// ImmediateFail is a policy the consumer chose, and the refusal it produces says + /// nothing about the server: the connection was being rebuilt and the caller asked not to + /// wait for it. Retrying once connected is the reaction. It used to arrive as the same + /// as "the reconnect loop gave up", which calls for a + /// failover instead. + /// + [TestMethod] + public async Task TestUARequestRefusedByPolicySaysSoRatherThanBlamingTheServer() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + // Issued from inside the switch, where the old socket is gone and the new one is not + // open yet - the window the policy exists for. + Exception refusal = null; + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && refusal == null) + { + try + { + await _client.Request(new Dictionary { { "command", "server_info" } }); + } + catch (Exception error) + { + refusal = error; + } + } + }; + + await _client.connection.ChangeServer($"ws://127.0.0.1:{secondPort}"); + + Assert.IsInstanceOfType( + refusal, + $"A request refused by ImmediateFail must say so, got: {refusal?.GetType().Name ?? "no exception"}."); + Assert.IsInstanceOfType(refusal, "catch (NotConnectedException) must keep catching this."); + } + finally + { + first.Stop(); + second.Stop(); + } + } + + /// + /// Switching servers survives a connection that settles on the second try, the way + /// connecting does. + /// + /// + /// + /// Connect() is two operations - the connection, and the server_info that + /// reads the network id - and it has carried the second across a teardown since 11.4.0, + /// because a socket really does open for a moment before a failing handler brings it down. + /// ChangeServer is the same two operations against a different server and had no + /// such protection: it read the network id once, directly, so a connection that needed a + /// second attempt failed the switch. + /// + /// + /// The second server drops the connection the first time it is asked for + /// server_info and serves normally afterwards, which puts the teardown exactly + /// between "the switch is connected" and "the switch has read the network id" - the only + /// window where the two behave differently. + /// + /// + [TestMethod] + public async Task TestUSwitchingServersSurvivesAConnectionThatSettlesOnRetry() + { + int firstPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + DropsFirstServerInfoServer dropping = new DropsFirstServerInfoServer(); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 4, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(15), + UseCustomPing = false, + }); + + await _client.Connect(); + + await _client.ChangeServer(dropping.Url); + + Assert.IsTrue(_client.connection.IsConnected(), "The switch has to end connected, not thrown."); + } + finally + { + first.Stop(); + dropping.Dispose(); + } + } + + /// + /// A request swept by a server switch is told which switch took the connection, and where. + /// + /// + /// + /// This is the failure consumers meet most often - not an overtaken transition, but their + /// own request dying while the connection moved underneath it. It arrived as a bare + /// reading "Connection was intentionally closed", + /// identical to a request the caller cancelled itself. + /// + /// + /// The new type still derives from , so a caller + /// that treated this as cancellation keeps working unchanged and the task keeps the status + /// it had. That is asserted here alongside the new information, because it is the promise + /// that makes the change safe to ship in a minor version. + /// + /// + [TestMethod] + public async Task TestUARequestSweptByASwitchNamesTheSwitch() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + + CreateMockRippled first = new CreateMockRippled(firstPort) { suppressOutput = true }; + first.AddResponse("server_info", ServerInfoResponse()); + // Sat on long enough that the switch below lands while the request is still in flight. + first.AddDelayedResponse("ledger", ServerInfoResponse(), TimeSpan.FromSeconds(10)); + new Thread(() => first.Start()) { IsBackground = true }.Start(); + + CreateMockRippled second = StartMock(secondPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + RequestTimeout = TimeSpan.FromSeconds(30), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + Task inFlight = _client.Request(new Dictionary { { "command", "ledger" } }); + await Task.Delay(TimeSpan.FromMilliseconds(300)); // it is on the wire, and unanswered + + string secondUrl = $"ws://127.0.0.1:{secondPort}"; + await _client.connection.ChangeServer(secondUrl); + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await inFlight); + + Assert.AreEqual(ConnectionTransitionKind.ChangeServer, error.Kind); + Assert.AreEqual(secondUrl, error.SupersededBy); + Assert.IsInstanceOfType( + error, + "catch (OperationCanceledException) must keep catching a swept request."); + } + finally + { + first.Stop(); + second.Stop(); + } + } + + /// + /// The notification that says the client stopped also says why it stopped. + /// + /// + /// + /// Disconnected is announced from ten places - a consumer's disconnect, a first + /// connection that failed, a connection closed for good, a broken handler, and the reconnect + /// loop running out of budget - and none of them carried anything but text. "Still trying" + /// against "gave up" was derivable only from the absence of ReconnectInfo, which is + /// also what a client that never had a loop looks like. + /// + /// + /// The reason goes on the notification rather than into ReconnectInfo: filling that + /// in on a terminal notification would take Reconnect != null, which consumers read + /// as "a loop is running", and give it a second meaning. It stays null here, and that is + /// asserted. + /// + /// + [TestMethod] + public async Task TestUTheTerminalNotificationSaysWhyTheClientStopped() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + List statuses = new List(); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + await _client.Connect(); + + _client.connection.OnConnectionStatus += status => + { + lock (statuses) + { + statuses.Add(status); + } + }; + + int deadPort = TestUtils.GetFreePort(); + try + { + await _client.connection.ChangeServer($"ws://127.0.0.1:{deadPort}"); + } + catch (ReconnectExhaustedException) + { + // The subject of this test is the notification, not the exception. + } + + ConnectionStatusInfo terminal; + lock (statuses) + { + terminal = statuses.FindLast(s => s.ConnectionState == XrpConnectionState.Disconnected); + } + + Assert.IsNotNull(terminal, "The client has to announce that it stopped."); + Assert.AreEqual( + ConnectionStopReason.ReconnectExhausted, + terminal.StopReason, + "Giving up on the budget is a different event from the consumer disconnecting."); + Assert.IsNull( + terminal.Reconnect, + "Reconnect stays null on a terminal notification, so 'a loop is running' keeps its one meaning."); + } + finally + { + mock.Stop(); + } + } + + /// + /// A consumer's own disconnect is named as such, and is not confused with giving up. + /// + [TestMethod] + public async Task TestUAConsumerDisconnectIsNamedInTheStatusStream() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + List statuses = new List(); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + _client.connection.OnConnectionStatus += status => + { + lock (statuses) + { + statuses.Add(status); + } + }; + + XrplClient disconnected = _client; + await _client.Disconnect(); + _client = null; + + ConnectionStatusInfo terminal; + lock (statuses) + { + terminal = statuses.FindLast(s => s.ConnectionState == XrpConnectionState.Disconnected); + } + + Assert.IsNotNull(terminal); + Assert.AreEqual(ConnectionStopReason.UserDisconnected, terminal.StopReason); + + // And the wait says the same, under the same name. The two enums are two views of + // one event, and the pair for the most ordinary ending of all was the one the + // suite never asked for - it was also the pair whose names did not match. + ConnectionWaitOutcome outcome = + await disconnected.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5)); + + Assert.AreEqual(ConnectionWaitOutcome.UserDisconnected, outcome); + } + finally + { + mock.Stop(); + } + } + + /// + /// "Did it come back?" is answerable without catching anything - and the answer says which + /// of the ways it did not. + /// + /// + /// + /// Both known consumers wrapped the throwing wait to get a value back, because "it did not + /// come back in time" is an answer a caller has to act on rather than an exceptional event. + /// A bool would have been the obvious shape and the wrong one: it folds "timed out", + /// "gave up" and "nothing is running" into one false, which is the problem this + /// whole change is about, moved into a new method. + /// + /// + /// The caller's own cancellation and an invalid timeout stay exceptions: the first is the + /// .NET convention, the second is a mistake by the caller rather than an outcome of the + /// connection. + /// + /// + [TestMethod] + public async Task TestUTheOutcomeOfAWaitIsAValueAndNamesTheCase() + { + int port = TestUtils.GetFreePort(); + _client = new XrplClient($"ws://127.0.0.1:{port}"); + + ConnectionWaitOutcome outcome = + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(1)); + + Assert.AreEqual(ConnectionWaitOutcome.NotConnecting, outcome); + } + + /// + /// It is callable through the interface and through the class alike. + /// + /// + /// The interface member is defaulted - forwarding to connection is the only + /// implementation that means anything, and an external implementer of + /// should not have to add one. A default member is only visible + /// through an interface-typed reference, though, so the client carries its own as well; + /// both are exercised here because a consumer holding either must be able to ask. + /// + [TestMethod] + public async Task TestUTheOutcomeIsReachableThroughTheInterfaceAndTheClass() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + IXrplClient asInterface = _client; + + Assert.AreEqual( + ConnectionWaitOutcome.Connected, + await asInterface.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5))); + Assert.AreEqual( + ConnectionWaitOutcome.Connected, + await _client.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5))); + } + finally + { + mock.Stop(); + } + } + + /// + /// A request swept by the client rebuilding its own connection names that rebuild. + /// + /// + /// The fourth kind, and the one a consumer must not read as a failure of the node: the + /// health check found the connection silent and replaced it, the server is where it always + /// was, and the request is worth sending again once the new connection is up. Reached + /// through the health check because that is the only path that rebuilds a connection + /// nobody asked it to rebuild. + /// + [TestMethod] + public async Task TestUARequestSweptByAReconnectNamesTheReconnect() + { + using SilentOnPingAndLedgerServer silent = new SilentOnPingAndLedgerServer(); + + _client = new XrplClient(silent.Url, new XrplClient.ClientOptions + { + RequestTimeout = TimeSpan.FromSeconds(30), + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(500), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(3), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = true, + HealthCheckInterval = TimeSpan.FromMilliseconds(200), + InactivityTimeout = TimeSpan.FromMilliseconds(500), + }); + + await _client.Connect(); + + Task inFlight = _client.Request(new Dictionary { { "command", "ledger" } }); + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await inFlight); + + Assert.AreEqual(ConnectionTransitionKind.Reconnect, error.Kind); + Assert.IsInstanceOfType(error); + } + + /// + /// A waiter parked while the client gives up on a broken handler is answered, not left to + /// time out. + /// + /// + /// + /// Two things have to line up for this to work, and neither is obvious. The give-up + /// announces itself before it performs the disconnect that records why, so a waiter woken + /// by that announcement finds nothing terminal yet and parks again. What has to reach it + /// then is the disconnect's own notification - and that one says Disconnected on a + /// client already reported as Disconnected, so the status stream suppresses it as a + /// duplicate. + /// + /// + /// Waking waiters therefore cannot be a side effect of emitting a status event: whether an + /// event is worth showing a consumer and whether the state changed are different + /// questions. The timeout here is long against a give-up that takes well under a second, + /// so "answered" and "gave up waiting" cannot be confused. + /// + /// + [TestMethod] + public async Task TestUAWaiterIsAnsweredWhenTheClientGivesUpOnItsHandler() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(20), + UseCustomPing = false, + }); + + _client.connection.OnConnected += () => throw new InvalidOperationException("permanently broken"); + + // Parked from the status stream rather than after a sleep: the socket is open for + // the moment the handler runs in, so a waiter started by the clock can find the + // client connected and answer that instead. RestoringConnection is the state where + // there is no socket and the client is between attempts - which is where a real + // caller waiting for the connection to come back sits. + Task waiting = null; + _client.connection.OnConnectionStatus += status => + { + if (status.ConnectionState == XrpConnectionState.RestoringConnection + && Interlocked.CompareExchange(ref waiting, null, null) == null) + { + Interlocked.CompareExchange( + ref waiting, + _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(20)), + null); + } + }; + + System.Diagnostics.Stopwatch clock = System.Diagnostics.Stopwatch.StartNew(); + + await Assert.ThrowsExactlyAsync(async () => await _client.Connect()); + + Task parked = Interlocked.CompareExchange(ref waiting, null, null); + Assert.IsNotNull(parked, "Precondition: the client has to report RestoringConnection at least once."); + + ConnectionWaitOutcome outcome = await parked; + clock.Stop(); + + // Not asserted as one particular outcome: the socket really does open for the + // moment the handler runs in, so a waiter can legitimately be answered + // "Connected" by an attempt that is about to fail, or "ConnectHandlerFailed" by + // the give-up. Which one wins is timing. What must never happen is neither - a + // waiter left parked because the wake-up that concerned it was swallowed. + Assert.AreNotEqual( + ConnectionWaitOutcome.TimedOut, + outcome, + "The waiter sat out its whole timeout: a state change it was waiting for did not reach it."); + Assert.IsLessThan( + TimeSpan.FromSeconds(10), + clock.Elapsed, + "The waiter has to be answered by the client, not by its own deadline."); + } + finally + { + mock.Stop(); + } + } + + /// + /// A failure the client is about to retry names no reason for stopping, because it has not + /// stopped. + /// + /// + /// + /// is defined as why the client stopped, and + /// as "this notification is not an ending". A + /// reason on a notification that is followed by a reconnect breaks that definition in the + /// way that costs something: a consumer reading any reason as terminal fails over to + /// another server while this one is still being dialled. + /// + /// + /// The first handshake against a server that is not up is exactly that case - it is + /// reported, and then retried. Whether a reason is named now follows the same condition + /// that decides whether the retry happens, so the two cannot say different things. + /// + /// + [TestMethod] + public async Task TestUAFailureTheClientWillRetryNamesNoStopReason() + { + int port = TestUtils.GetFreePort(); // nothing is listening, and nothing will be + + List statuses = new List(); + + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 1000, + StopAfterMaxAttempts = false, // it will keep trying, so it never stops + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(3), + UseCustomPing = false, + }); + + _client.connection.OnConnectionStatus += status => + { + lock (statuses) + { + statuses.Add(status); + } + }; + + try + { + await _client.Connect(); + } + catch (Exception) + { + // The connection never comes up; the subject here is what was announced on the way. + } + + await Task.Delay(TimeSpan.FromMilliseconds(500)); + + List claimingAnEnding; + bool stillTrying; + string trace; + lock (statuses) + { + claimingAnEnding = statuses.FindAll(s => s.StopReason != ConnectionStopReason.None); + stillTrying = statuses.Exists(s => s.ConnectionState == XrpConnectionState.RestoringConnection); + trace = string.Join(" | ", statuses.ConvertAll(x => $"{x.ConnectionState}/{x.StopReason}")); + } + + Assert.IsTrue(stillTrying, $"Precondition: the client has to be retrying. Sequence was: {trace}"); + Assert.AreEqual( + 0, + claimingAnEnding.Count, + $"A client that is still dialling must not name a reason for having stopped. Sequence was: {trace}"); + } + + /// + /// A client that spent its reconnect budget stays stopped. + /// + /// + /// + /// StopAfterMaxAttempts is a promise that the client will stop asking, and the + /// consumer is told it has: Disconnected with + /// . A second series behind that + /// notification breaks the promise twice over - the budget is spent again without being + /// granted again, and a consumer that failed over on the first notification now has a + /// client quietly dialling the endpoint it moved away from. + /// + /// + /// What made it possible: the loop's exit clears the two fields that say "a sequence is + /// running for this generation", which is exactly what "no sequence is running" looks + /// like. The close of the last failed attempt arrives after that and is indistinguishable + /// from the first one. + /// + /// + [TestMethod] + public async Task TestUAClientThatSpentItsReconnectBudgetStaysStopped() + { + SilentOnPingAndLedgerServer silent = new SilentOnPingAndLedgerServer(); + + List statuses = new List(); + + try + { + _client = new XrplClient(silent.Url, new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 1, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(1), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(5), + UseCustomPing = false, + }); + + await _client.Connect(); + + _client.connection.OnConnectionStatus += status => + { + lock (statuses) + { + statuses.Add(status); + } + }; + + silent.Dispose(); + + // Long enough for a second series to have run and announced itself: the budget + // above is spent in a few hundred milliseconds. + await Task.Delay(TimeSpan.FromSeconds(4)); + + int gaveUp; + string trace; + lock (statuses) + { + gaveUp = statuses.FindAll(s => + s.ConnectionState == XrpConnectionState.Disconnected && + s.StopReason == ConnectionStopReason.ReconnectExhausted).Count; + trace = string.Join(" | ", statuses.ConvertAll(x => $"{x.ConnectionState}/{x.StopReason}: {x.Message}")); + } + + Assert.AreEqual(1, gaveUp, $"Giving up is announced once and meant once. Sequence was: {trace}"); + Assert.IsFalse(_client.connection.IsConnected()); + + // The other half of the promise, and the risk of keeping it: stopping must not mean + // wedged. Asking again is the consumer's decision, and a consumer command begins a + // new generation - which is what lifts the refusal, with no flag to reset and no + // way for it to outlive the sequence it belongs to. + int livePort = TestUtils.GetFreePort(); + CreateMockRippled live = StartMock(livePort); + try + { + await _client.ChangeServer($"ws://127.0.0.1:{livePort}"); + Assert.IsTrue( + _client.connection.IsConnected(), + "A client that stopped asking on its own must still answer the consumer asking."); + } + finally + { + live.Stop(); + } + } + finally + { + silent.Dispose(); + } + } + + /// + /// The same answer whether the handler fails at once or a moment later. + /// + /// + /// + /// The timing decides which path reports the failure. A handler that throws at once leaves + /// nothing in flight and the caller is answered by the wait; one that works for a moment + /// first - a subscribe that gets some way in before falling over - lets the caller reach + /// the network-id read, and the teardown then finds a request in flight to reject. Both + /// used to be the same bare exception, and attaching the reason to only one of them would + /// have handed the caller two different types for one scenario depending on how fast the + /// machine was. + /// + /// + /// Holding server_info back is what makes the second path certain rather than lucky: + /// answered at once, the request is gone before the handler gives up and the test would + /// pass without ever exercising the case. + /// + /// + [TestMethod] + public async Task TestUABrokenConnectHandlerAnswersTheSameWhicheverWayItFails() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = new CreateMockRippled(port) { suppressOutput = true }; + mock.AddDelayedResponse("server_info", ServerInfoResponse(), TimeSpan.FromSeconds(5)); + new Thread(() => mock.Start()) { IsBackground = true }.Start(); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 1, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + InvalidOperationException thrownByHandler = new InvalidOperationException("broken, but not straight away"); + _client.connection.OnConnected += async () => + { + await Task.Delay(TimeSpan.FromMilliseconds(400)); + throw thrownByHandler; + }; + + ConnectHandlerFailedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.Connect()); + + Assert.AreSame(thrownByHandler, error.InnerException); + Assert.IsGreaterThanOrEqualTo(1, error.Failures); + } + finally + { + mock.Stop(); + } + } + + /// + /// The status stream names a broken handler as such, and says nothing about a notification + /// that is not terminal. + /// + /// + /// The two halves belong together: a reason that appeared on every notification would be + /// as useless as none at all, and on the + /// RestoringConnection that precedes the give-up is what lets a consumer treat the + /// field as "this one is terminal, and here is why". + /// + [TestMethod] + public async Task TestUTheStatusStreamNamesABrokenHandlerAndOnlyWhenTerminal() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + List statuses = new List(); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + _client.connection.OnConnectionStatus += status => + { + lock (statuses) + { + statuses.Add(status); + } + }; + + _client.connection.OnConnected += () => throw new InvalidOperationException("permanently broken"); + + try + { + await _client.Connect(); + } + catch (ConnectHandlerFailedException) + { + // The subject here is the status stream. + } + + ConnectionStatusInfo terminal; + List nonTerminal; + lock (statuses) + { + terminal = statuses.FindLast(s => s.ConnectionState == XrpConnectionState.Disconnected); + nonTerminal = statuses.FindAll(s => s.ConnectionState != XrpConnectionState.Disconnected); + } + + string seq; + lock (statuses) + { + seq = string.Join(" | ", statuses.ConvertAll(x => $"{x.ConnectionState}/{x.StopReason}")); + } + + Assert.IsNotNull(terminal); + + // The last word, deliberately: the give-up announces the handler failure and then + // performs a disconnect of its own, which announces again. Both have to name the + // same event, or the status stream ends by contradicting the exception the same + // failure produced. Reading the last one is what catches that. + Assert.AreEqual( + ConnectionStopReason.ConnectHandlerFailed, + terminal.StopReason, + $"The last thing said about a broken handler must still be the handler. Sequence was: {seq}"); + + foreach (ConnectionStatusInfo status in nonTerminal) + { + Assert.AreEqual( + ConnectionStopReason.None, + status.StopReason, + $"A {status.ConnectionState} notification is not an ending and must not claim a reason."); + } + } + finally + { + mock.Stop(); + } + } + + /// + /// A switch overtaken at the client level is reported, not retried away. + /// + /// + /// XrplClient.ChangeServer reads the network id after the switch and carries that + /// read across a teardown, which means it has a retry loop around an operation that can + /// fail because the client moved. The loop must not swallow a supersession: the caller's + /// switch did not happen, and asking again would read the network id of a server it never + /// named. The line is drawn by the kind of transition, which is why this is asserted + /// through the client rather than through the connection. + /// + [TestMethod] + public async Task TestUAnOvertakenSwitchIsReportedThroughTheClientToo() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + int thirdPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + CreateMockRippled third = StartMock(thirdPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + string thirdUrl = $"ws://127.0.0.1:{thirdPort}"; + int nested = 0; + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && Interlocked.Exchange(ref nested, 1) == 0) + { + await _client.connection.ChangeServer(thirdUrl); + } + }; + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await _client.ChangeServer($"ws://127.0.0.1:{secondPort}")); + + Assert.AreEqual(ConnectionTransitionKind.ChangeServer, error.Kind); + Assert.AreEqual(thirdUrl, error.SupersededBy); + } + finally + { + first.Stop(); + second.Stop(); + third.Stop(); + } + } + + /// + /// Starts a mock that sits on one command, so a request can still be in flight when + /// something happens to the connection. + /// + private static CreateMockRippled StartMockHoldingOnto(int port, string command, TimeSpan delay) + { + CreateMockRippled mock = new CreateMockRippled(port) { suppressOutput = true }; + mock.AddResponse("server_info", ServerInfoResponse()); + mock.AddDelayedResponse(command, ServerInfoResponse(), delay); + + new Thread(() => mock.Start()) { IsBackground = true }.Start(); + return mock; + } + + private XrplClient ClientHoldingRequests(int port) => + new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + RequestTimeout = TimeSpan.FromSeconds(30), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + /// + /// A request swept by the consumer disconnecting is told so, and is still a cancellation. + /// + /// + /// The kind matters more here than anywhere: a request that died because the consumer took + /// the client down needs no retry and no failover, and it used to be indistinguishable from + /// one that died because the connection moved. It stays an + /// rather than becoming a + /// - the request was cancelled, and changing that + /// would turn a cancellation into a failure for every caller that already handles it. + /// + [TestMethod] + public async Task TestUARequestSweptByADisconnectNamesTheDisconnect() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMockHoldingOnto(port, "ledger", TimeSpan.FromSeconds(10)); + + try + { + _client = ClientHoldingRequests(port); + await _client.Connect(); + + Task inFlight = _client.Request(new Dictionary { { "command", "ledger" } }); + await Task.Delay(TimeSpan.FromMilliseconds(300)); + + await _client.Disconnect(); + _client = null; + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await inFlight); + + Assert.AreEqual(ConnectionTransitionKind.Disconnect, error.Kind); + Assert.IsNull(error.SupersededBy, "A disconnect took the client nowhere."); + Assert.IsInstanceOfType(error); + } + finally + { + mock.Stop(); + } + } + + /// + /// Connect() on a client that is already connected disturbs nothing. + /// + /// + /// Written while trying to reach the sweep that Connect() performs, and kept because + /// of what it found instead: Connect() returns at once when the client is already + /// connected, so it never gets as far as taking the socket over. The sweep is therefore + /// reachable only from a client that is not connected - where there is no request in flight + /// to sweep - and the Connect kind is exercised through supersession instead. What + /// is worth pinning here is that a redundant Connect() does not quietly kill the + /// requests a caller has outstanding. + /// + [TestMethod] + public async Task TestURedundantConnectDoesNotDisturbRequestsInFlight() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMockHoldingOnto(port, "ledger", TimeSpan.FromSeconds(1)); + + try + { + _client = ClientHoldingRequests(port); + await _client.Connect(); + + Task inFlight = _client.Request(new Dictionary { { "command", "ledger" } }); + await Task.Delay(TimeSpan.FromMilliseconds(200)); + + await _client.connection.Connect(CancellationToken.None); + + await inFlight; // answered by the connection it was written to + Assert.IsTrue(_client.connection.IsConnected()); + } + finally + { + mock.Stop(); + } + } + + /// + /// A request killed by the connection failing on its own stays a plain cancellation. + /// + /// + /// + /// This is the deliberate border of the change. A connection that failed has no transition + /// to name and no destination to point at, so inventing one would be worse than saying + /// little: a consumer switching on would be told the + /// client moved somewhere when nothing moved it. + /// + /// + /// What such a request actually gets is , reported by + /// the close path with the code and reason the peer gave - measured here rather than + /// assumed, because the sweep this test was written to check turned out not to be the one + /// that answers first. The assertion that matters either way is the negative one: no + /// transition is named. + /// + /// + [TestMethod] + public async Task TestUARequestKilledByANetworkDropNamesNoTransition() + { + DropsFirstServerInfoServer dropping = new DropsFirstServerInfoServer(); + + try + { + _client = new XrplClient(dropping.Url, new XrplClient.ClientOptions + { + RequestTimeout = TimeSpan.FromSeconds(30), + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + MaxReconnectAttempts = 4, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + // The connection alone, so nothing asks for server_info before the test does. + await _client.connection.Connect(CancellationToken.None); + + Exception failure = null; + try + { + await _client.Request(new Dictionary { { "command", "server_info" } }); + } + catch (Exception error) + { + failure = error; + } + + Assert.IsNotNull(failure, "The request died with the connection and has to say so."); + Assert.IsNotInstanceOfType( + failure, + "No transition took this connection anywhere - it failed. Naming a transition would be inventing one."); + Assert.IsInstanceOfType( + failure, + $"A peer that went away is reported by the close path, got: {failure.GetType().Name}."); + } + finally + { + dropping.Dispose(); + } + } + + /// + /// Starts a client that will keep trying to reach a server that is not there, so a wait + /// actually parks instead of being answered on entry. + /// + /// + /// An attempt has to be in progress for the wait to reach its parking spot at all: with + /// nothing running it is answered by straight away, + /// which is a different test. The connect task is left running on purpose and torn down by + /// the cleanup. + /// + private XrplClient StartClientWaitingForever(int port) + { + XrplClient client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 1000, + StopAfterMaxAttempts = false, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(60), + UseCustomPing = false, + }); + + _ = client.Connect(); + return client; + } + + /// + /// Waiting for a connection that never comes: the timeout is an answer, the caller's own + /// cancellation is not, and a bad timeout is the caller's mistake. + /// + /// + /// These three are the boundary of the outcome contract and the reason it is not simply + /// "every failure becomes a value". A timeout is something the caller has to act on and so + /// it is reported as ; cancellation through the + /// caller's own token stays an exception because that is the .NET convention and because + /// the caller already knows it cancelled; an invalid timeout is a bug at the call site + /// rather than an outcome of the connection. This is also the code that changed most when + /// the wait stopped polling, so it is pinned rather than assumed. + /// + [TestMethod] + public async Task TestUTimeoutIsAnAnswerAndCancellationIsNot() + { + int port = TestUtils.GetFreePort(); // nothing is listening, and nothing will be + _client = StartClientWaitingForever(port); + + // The wait has to be parked, not answered on entry. + await Task.Delay(TimeSpan.FromMilliseconds(300)); + + Assert.AreEqual( + ConnectionWaitOutcome.TimedOut, + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromMilliseconds(300)), + "Not coming back in time is an answer, not a failure."); + + using CancellationTokenSource alreadyCancelled = new CancellationTokenSource(); + alreadyCancelled.Cancel(); + await Assert.ThrowsAsync(async () => + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(30), alreadyCancelled.Token)); + + using CancellationTokenSource cancelledWhileWaiting = new CancellationTokenSource(); + Task waiting = + _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(30), cancelledWhileWaiting.Token); + cancelledWhileWaiting.CancelAfter(TimeSpan.FromMilliseconds(200)); + await Assert.ThrowsAsync(async () => await waiting); + + await Assert.ThrowsExactlyAsync(async () => + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.Zero)); + } + + /// + /// Waiters do not interfere: one timing out and one cancelling leave the third waiting. + /// + /// + /// The wait sleeps on a signal shared by everyone waiting, so a per-waiter deadline must + /// not touch it: an implementation that cancelled the shared signal to serve its own + /// timeout would take every other waiter down with it, and one that never re-armed the + /// signal after completing it would leave the survivors awake and spinning. Both are the + /// kind of mistake that shows only with more than one waiter, which is why this test has + /// three. + /// + [TestMethod] + public async Task TestUWaitersDoNotTakeEachOtherDown() + { + int port = TestUtils.GetFreePort(); + _client = StartClientWaitingForever(port); + + await Task.Delay(TimeSpan.FromMilliseconds(300)); + + using CancellationTokenSource cancelling = new CancellationTokenSource(); + + Task timesOut = + _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromMilliseconds(400)); + Task cancelled = + _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(30), cancelling.Token); + Task survives = + _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(30)); + + cancelling.CancelAfter(TimeSpan.FromMilliseconds(200)); + + Assert.AreEqual(ConnectionWaitOutcome.TimedOut, await timesOut); + await Assert.ThrowsAsync(async () => await cancelled); + + Assert.IsFalse(survives.IsCompleted, "The third waiter has no reason to be finished yet."); + + // And it is still a live waiter rather than a spinning one: the server comes up, and it + // is the connection that ends the wait. + CreateMockRippled mock = StartMock(port); + try + { + Assert.AreEqual(ConnectionWaitOutcome.Connected, await survives); + } + finally + { + mock.Stop(); + } + } + + /// + /// A transition a Disconnect() overtook is told the client is down, not that it was + /// overtaken. + /// + /// + /// The two answers call for opposite reactions, which is why the disconnect branch keeps + /// reporting a while every other winner reports a + /// cancellation: after a Disconnect() nothing is coming back on its own, and a + /// consumer that treated this as "the switch was overtaken, carry on" would be waiting for + /// a connection nobody is building. + /// + [TestMethod] + public async Task TestUASwitchADisconnectOvertookIsToldTheClientIsDown() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + int disconnected = 0; + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && Interlocked.Exchange(ref disconnected, 1) == 0) + { + await _client.Disconnect(); + } + }; + + ClientDisconnectedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{secondPort}")); + + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + Assert.IsFalse(_client.connection.IsConnected(), "The disconnect won, and it has to stay won."); + _client = null; + } + finally + { + first.Stop(); + second.Stop(); + } + } + + /// + /// A switch overtaken while it waits for its own connection reports it too. + /// + /// + /// This is the check that runs after the wait rather than before it, and it is reached by a + /// different route than the others: the switch connected, and only then found the client + /// somewhere else. Its condition can prove only that the client is not where this call + /// asked for, so the kind is read from the transition that owns the connection instead of + /// being assumed from the mismatch - and no existing test in this repository passed through + /// it at all. + /// + [TestMethod] + public async Task TestUASwitchOvertakenWhileWaitingReportsTheWinner() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + int thirdPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + CreateMockRippled third = StartMock(thirdPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + string thirdUrl = $"ws://127.0.0.1:{thirdPort}"; + string secondUrl = $"ws://127.0.0.1:{secondPort}"; + + // Issued from the new server's own OnConnected, which runs while the first switch is + // still inside its wait - so the takeover lands after the connection came up and + // before the switch checks where it ended up. + int nested = 0; + _client.connection.OnConnected += async () => + { + if (string.Equals(_client.connection.GetUrl(), secondUrl, StringComparison.Ordinal) + && Interlocked.Exchange(ref nested, 1) == 0) + { + await _client.connection.ChangeServer(thirdUrl); + } + }; + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer(secondUrl)); + + Assert.AreEqual(thirdUrl, error.SupersededBy, "The caller is told where the client actually is."); + Assert.AreEqual(ConnectionTransitionKind.ChangeServer, error.Kind); + } + finally + { + first.Stop(); + second.Stop(); + third.Stop(); + } + } + + /// + /// A Connect() that overtakes a switch is named as a Connect(). + /// + /// + /// The kind is what a consumer reacts to: another ChangeServer put the client on a + /// server it did not ask for, while a Connect() rebuilt the connection to the one it + /// was already heading for. Both were the same bare cancellation before. + /// + [TestMethod] + public async Task TestUASwitchAConnectOvertookNamesTheConnect() + { + int firstPort = TestUtils.GetFreePort(); + int secondPort = TestUtils.GetFreePort(); + + CreateMockRippled first = StartMock(firstPort); + CreateMockRippled second = StartMock(secondPort); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{firstPort}", new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + await _client.Connect(); + + int reconnected = 0; + _client.OnSessionEnded += async (reason, _) => + { + if (reason == SessionEndReason.ServerChanged && Interlocked.Exchange(ref reconnected, 1) == 0) + { + await _client.connection.Connect(CancellationToken.None); + } + }; + + ConnectionSupersededException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{secondPort}")); + + Assert.AreEqual(ConnectionTransitionKind.Connect, error.Kind); + } + finally + { + first.Stop(); + second.Stop(); + } + } + + /// + /// A waiter learns that the client gave up as soon as it gives up, not when its own timeout + /// runs out. + /// + /// + /// + /// This guards the wait itself. While it polled, every terminal state was noticed within a + /// tick whether or not anything announced it; waiting on a signal instead means a terminal + /// condition that forgets to wake its waiters is not slow but invisible - the caller sits + /// out the whole acquisition timeout and is then told it timed out, which is the wrong + /// answer as well as a late one. + /// + /// + /// The acquisition timeout here is thirty seconds against a reconnect budget that is spent + /// in well under one, so the assertion can tell "woken by the client" from "gave up + /// waiting" without depending on how fast the machine is. + /// + /// + [TestMethod] + public async Task TestUAWaiterIsWokenWhenTheClientGivesUpNotWhenItsOwnTimeoutExpires() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + await _client.Connect(); + + int deadPort = TestUtils.GetFreePort(); + + System.Diagnostics.Stopwatch clock = System.Diagnostics.Stopwatch.StartNew(); + await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{deadPort}")); + clock.Stop(); + + Assert.IsLessThan( + TimeSpan.FromSeconds(15), + clock.Elapsed, + "The waiter has to be woken by the client giving up, not by its own 30s timeout."); + } + finally + { + mock.Stop(); + } + } + + /// + /// A caller who asks after the reconnect loop has given up is told the budget was spent - + /// the same thing the status stream told them a moment earlier. + /// + /// + /// + /// The natural shape of a failover is to hear + /// on the status stream and then + /// confirm it on the wait before switching servers. That caller arrives after the sequence + /// has ended - no socket, no cancellation source, a Disconnected state - which is also, + /// exactly, what a client nobody has called Connect() on looks like. The wait used + /// to answer that reading first, so the same client reported ReconnectExhausted or + /// NotConnecting depending only on whether the caller had happened to park before + /// the loop stopped, and the outcome this API exists to deliver was the one a consumer + /// could not get. + /// + /// + /// The assertion is made against the status stream rather than against a literal: what + /// matters is that the two ways of asking agree. + /// + /// + [TestMethod] + public async Task TestUTheWaitAndTheStatusStreamAgreeAfterTheBudgetIsSpent() + { + int deadPort = TestUtils.GetFreePort(); // nothing is listening there, and never will be + _client = new XrplClient($"ws://127.0.0.1:{deadPort}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(50), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(100), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromMilliseconds(500), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + TaskCompletionSource stopped = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + _client.connection.OnConnectionStatus += info => + { + if (info.StopReason == ConnectionStopReason.ReconnectExhausted) + { + stopped.TrySetResult(true); + } + }; + + try + { + await _client.Connect(); + } + catch (NotConnectedException) + { + // What this caller was told is not the subject; what a later one is told is. + } + + await stopped.Task.WaitAsync(TimeSpan.FromSeconds(20)); + + ConnectionWaitOutcome outcome = + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(1)); + + Assert.AreEqual( + ConnectionWaitOutcome.ReconnectExhausted, + outcome, + "The status stream said the budget was spent; a caller asking straight afterwards must be told the same."); + + ReconnectExhaustedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.WaitForConnectionAsync(TimeSpan.FromSeconds(1))); + + Assert.AreEqual(2, error.MaxAttempts, "The budget the client was configured with."); + } + + /// + /// A close code this client does not reconnect after ends the wait, rather than leaving it + /// to run out its timeout. + /// + /// + /// + /// 1002, 1003, 1007 and 1010 are the codes after which no reconnect is started at all - + /// the node is saying that retrying against it is pointless. The status stream has reported + /// that ending for as long as has + /// existed; the wait could not, because the ending leaves nothing behind to recognise it + /// by: no socket, no reconnect loop, and an attempt counter the close path resets to zero. + /// A caller already parked was woken by the announcement, found nothing, and parked again - + /// to be told, a whole acquisition timeout later, that the connection "was not established + /// in time" about a connection the consumer had already been told was over. + /// + /// + /// The waiter here arrives after the close, which is answered through the wait's entry; + /// the parked case is the same terminal reached through the wake. + /// + /// + [TestMethod] + public async Task TestUAPermanentCloseEndsTheWaitInsteadOfRunningItOut() + { + using ClosesWithCodeServer server = new ClosesWithCodeServer(closeCode: 1003); + + _client = new XrplClient(server.Url, new XrplClient.ClientOptions + { + ConnectionAttemptTimeout = TimeSpan.FromSeconds(5), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the server."); + + TaskCompletionSource closed = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + _client.connection.OnConnectionStatus += info => + { + if (info.StopReason == ConnectionStopReason.ClosedPermanently) + { + closed.TrySetResult(true); + } + }; + + server.CloseNow(); + await closed.Task.WaitAsync(TimeSpan.FromSeconds(20)); + + // A generous timeout on purpose: a wait that has to be answered by a terminal must not + // be seen to pass because its own deadline was short. + ConnectionClosedPermanentlyException error = + await Assert.ThrowsExactlyAsync( + async () => await _client.connection.WaitForConnectionAsync(TimeSpan.FromSeconds(30))); + + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + + ConnectionWaitOutcome outcome = + await _client.connection.WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(30)); + + Assert.AreEqual( + ConnectionWaitOutcome.ClosedPermanently, + outcome, + "The outcome and the stop reason are two views of one event and must name it the same."); + } + + /// + /// A wait whose timeout is an exact multiple of the system clock tick still ends on its own + /// deadline, and ends promptly. + /// + /// + /// Both readings of the clock are quantised to the tick - 15.625 ms on Windows - so the + /// elapsed time is always a whole number of ticks, and a timeout that is itself a whole + /// number of them is hit exactly rather than passed. A strict deadline comparison then + /// declined to fire while the remaining time was already zero, and the loop went round + /// again with nothing to await: a spin on the connection's own lock until the clock moved. + /// One second is 64 ticks exactly, as is the default acquisition timeout of five minutes. + /// + [TestMethod] + public async Task TestUAWaitWhoseTimeoutLandsOnAClockTickStillTimesOut() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromSeconds(30), + ReconnectMaxDelay = TimeSpan.FromSeconds(30), + MaxReconnectAttempts = 50, + StopAfterMaxAttempts = false, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(4), + UseCustomPing = false, + }); + + await _client.Connect(); + + int deadPort = TestUtils.GetFreePort(); + Task switching = _client.connection.ChangeServer($"ws://127.0.0.1:{deadPort}"); + + Stopwatch clock = Stopwatch.StartNew(); + await Assert.ThrowsExactlyAsync( + async () => await _client.connection.WaitForConnectionAsync(TimeSpan.FromSeconds(1))); + clock.Stop(); + + Assert.IsLessThan( + TimeSpan.FromSeconds(10), + clock.Elapsed, + "The wait has to end on its own deadline rather than spin past it."); + + try + { + await switching; + } + catch (Exception) + { + // The switch to a dead port is scenery; it never settles, and how it gives up + // is the subject of other tests. + } + } + finally + { + mock.Stop(); + } + } + + /// + /// A client that gave up comes back when the consumer asks it to, and the switch that asks + /// is not answered with the ending the previous one had. + /// + /// + /// + /// The whole point of a terminal ending is that the consumer decides what happens next, and + /// the decision they make is usually this one: move to another node. Recovery was covered + /// through Connect() and, on the WebAssembly stand, through ChangeServer; + /// this pins the second in the suite. What it guards against specifically is the wait + /// concluding, from state the previous sequence left behind, that a switch which has made + /// no attempts has already run out of them - every field it reads to decide that is keyed + /// by generation, and this is the test that says so. + /// + /// + [TestMethod] + public async Task TestUAClientThatGaveUpSwitchesToALiveServer() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + await _client.Connect(); + Assert.IsTrue(_client.connection.IsConnected(), "Precondition: connected to the mock."); + + int deadPort = TestUtils.GetFreePort(); // nothing is listening there, and never will be + + await Assert.ThrowsExactlyAsync( + async () => await _client.connection.ChangeServer($"ws://127.0.0.1:{deadPort}")); + + // The consumer's decision, and the client has to honour it rather than answer with + // the ending of the sequence that is over. + await _client.connection.ChangeServer($"ws://127.0.0.1:{port}"); + + Assert.IsTrue( + _client.connection.IsConnected(), + "A client that gave up must still switch to a server that answers."); + } + finally + { + mock.Stop(); + } + } + + /// + /// Every reason the client can stop for has exactly one outcome a waiter can be told, under + /// the same name. + /// + /// + /// + /// The two enums are two views of one event, and the defect they exist to remove comes back + /// the moment they stop corresponding: an ending announced on the status stream that the + /// wait has no way to express leaves a parked caller to sit out its whole timeout and be + /// told the connection "was not established in time" about a connection the consumer has + /// already been told was over. That happened twice - a close the client does not reconnect + /// after, and a first attempt with no retry behind it - and both were found by readers + /// rather than by this suite. + /// + /// + /// Asserted over the enum itself rather than over a list written out here, so that a reason + /// added later fails this test until it is given its counterpart. + /// is excluded: it is the absence of an ending. + /// + /// + [TestMethod] + public void TestUEveryStopReasonHasAnOutcomeOfTheSameName() + { + List missing = new List(); + + foreach (ConnectionStopReason reason in Enum.GetValues()) + { + if (reason == ConnectionStopReason.None) + { + continue; + } + + if (!Enum.TryParse(typeof(ConnectionWaitOutcome), reason.ToString(), ignoreCase: false, out object _)) + { + missing.Add(reason.ToString()); + } + } + + Assert.AreEqual( + 0, + missing.Count, + $"ConnectionStopReason values with no ConnectionWaitOutcome of the same name: {string.Join(", ", missing)}. " + + "A consumer told this on the status stream has no way to be told it by the wait."); + } + + /// + /// A request issued after the client gave up is told that it gave up, not that it was never + /// asked to connect. + /// + /// + /// The check every request passes through asks the same question the wait used to ask + /// first - is a socket or a reconnect source installed, and is the state anything other + /// than Disconnected - and a client that spent its budget answers no to all three, exactly + /// as a client nobody has called Connect() on does. So the status stream said the + /// endpoint had given up, the wait agreed, and a request on the same client raised + /// "no connection attempt in progress. Call Connect() first" - the one distinction this + /// exception family exists to draw, contradicted by the third way of asking. + /// + [TestMethod] + public async Task TestUARequestAfterTheClientGaveUpNamesGivingUp() + { + int deadPort = TestUtils.GetFreePort(); // nothing is listening there, and never will be + _client = new XrplClient($"ws://127.0.0.1:{deadPort}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(50), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(100), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromMilliseconds(500), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(10), + UseCustomPing = false, + }); + + TaskCompletionSource stopped = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + _client.connection.OnConnectionStatus += info => + { + if (info.StopReason == ConnectionStopReason.ReconnectExhausted) + { + stopped.TrySetResult(true); + } + }; + + try + { + await _client.Connect(); + } + catch (NotConnectedException) + { + // What this caller was told is not the subject. + } + + await stopped.Task.WaitAsync(TimeSpan.FromSeconds(20)); + + ReconnectExhaustedException error = await Assert.ThrowsExactlyAsync( + async () => await _client.connection.Request(new Dictionary + { + { "command", "server_info" }, + })); + + Assert.AreEqual(2, error.MaxAttempts, "The budget the client was configured with."); + Assert.IsInstanceOfType(error, "catch (NotConnectedException) must keep catching this."); + } + + /// + /// A client that has been announced as stopped does not report itself connected, however + /// open the socket it is still holding happens to be. + /// + /// + /// + /// The give-up path announces the ending, then rejects the requests in flight - which is + /// what resumes the caller inside Connect() - and only then disconnects. Between the + /// rejection and the disconnect the socket is still installed and still open, so a caller + /// asking in that moment was told the connection was up, one instant after being told why + /// it was over. The first failure shape there is: an operation reporting success while the + /// state it describes is gone. + /// + /// + /// The status notification is the pause point that makes the window deterministic rather + /// than raced. It is raised before the rejection, so inside the handler the socket is + /// guaranteed to be open and the ending is guaranteed to be recorded - the exact + /// interleaving that CI produced and this machine did not. + /// + /// + [TestMethod] + public async Task TestUAStoppedClientDoesNotCallItselfConnected() + { + int port = TestUtils.GetFreePort(); + CreateMockRippled mock = StartMock(port); + + try + { + _client = new XrplClient($"ws://127.0.0.1:{port}", new XrplClient.ClientOptions + { + ReconnectBaseDelay = TimeSpan.FromMilliseconds(100), + ReconnectMaxDelay = TimeSpan.FromMilliseconds(200), + MaxReconnectAttempts = 2, + StopAfterMaxAttempts = true, + ConnectionAttemptTimeout = TimeSpan.FromSeconds(2), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(30), + UseCustomPing = false, + }); + + _client.connection.OnConnected += () => throw new InvalidOperationException("handler is permanently broken"); + + bool? socketWasStillOpen = null; + ConnectionWaitOutcome? answeredInsideTheWindow = null; + + _client.connection.OnConnectionStatus += status => + { + if (status.StopReason != ConnectionStopReason.ConnectHandlerFailed || + answeredInsideTheWindow != null) + { + return; + } + + socketWasStillOpen = _client.connection.IsConnected(); + answeredInsideTheWindow = _client.connection + .WaitForConnectionOutcomeAsync(TimeSpan.FromSeconds(5)) + .GetAwaiter() + .GetResult(); + }; + + await Assert.ThrowsExactlyAsync(async () => await _client.Connect()); + + Assert.IsNotNull(answeredInsideTheWindow, "The terminal notification has to be raised for this to test anything."); + + Assert.AreEqual( + ConnectionWaitOutcome.ConnectHandlerFailed, + answeredInsideTheWindow, + socketWasStillOpen == true + ? "Asked while the socket was still open, and the client had already been announced as stopped." + : "Asked after the socket had closed, so this run did not exercise the window - but the answer is the same either way."); + } + finally + { + mock.Stop(); + } + } + + } +} diff --git a/Xrpl/Client/ConnectionManager.cs b/Xrpl/Client/ConnectionManager.cs index 1b5291f0..dbeb9106 100644 --- a/Xrpl/Client/ConnectionManager.cs +++ b/Xrpl/Client/ConnectionManager.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Threading.Tasks; @@ -6,45 +6,92 @@ namespace Xrpl.Client { + /// + /// Holds the callers waiting for the connection to come up, and releases them when it does. + /// + /// + /// + /// The connection notifies this from its socket, reconnect and caller threads, while consumers + /// register from theirs: client.connection.connectionManager is reachable from outside + /// the library. The list is therefore shared state and is guarded as such - it was not, and a + /// registration landing inside a notification threw + /// ("Collection was modified") or, worse, was dropped + /// by the reassignment that followed and never resumed. + /// + /// + /// Waiters are released outside the lock and resume asynchronously. Both matter for the same + /// reason: is called from inside the connection's own critical + /// path, before the OnConnected handler, and a waiter resuming there would run consumer + /// code in the middle of a connection being assembled. + /// + /// public class ConnectionManager { - private List<(Action resolve, Action reject)> PromisesAwaitingConnection = new List<(Action resolve, Action reject)>(); + private readonly object _waitersLock = new object(); - public void ResolveAllAwaiting() + private List> _promisesAwaitingConnection = new List>(); + + /// + /// Takes the waiters out, leaving an empty list behind for anyone registering next. + /// + private List> TakeWaiters() { - foreach (var (resolve, _) in PromisesAwaitingConnection) + lock (_waitersLock) { - resolve(); + List> waiting = _promisesAwaitingConnection; + _promisesAwaitingConnection = new List>(); + return waiting; } + } - PromisesAwaitingConnection = new List<(Action resolve, Action reject)>(); + /// Releases everyone waiting: the connection is up. + public void ResolveAllAwaiting() + { + foreach (TaskCompletionSource waiter in TakeWaiters()) + { + waiter.TrySetResult(null); + } } + /// Fails everyone waiting with . public void RejectAllAwaiting(Exception error) { - foreach (var (_, reject) in PromisesAwaitingConnection) + foreach (TaskCompletionSource waiter in TakeWaiters()) { - reject(error); + waiter.TrySetException(error); } - - PromisesAwaitingConnection = new List<(Action resolve, Action reject)>(); } + /// + /// Cancels everyone waiting, for a connection that was closed on purpose. + /// + /// + /// TrySetCanceled rather than an handed to + /// TrySetException: the second produces a faulted task, and the rule that turns a + /// cancellation into a cancelled task belongs to the async method builder rather than to + /// . + /// public void RejectAllAwaitingWithCancellation() { - foreach (var (_, reject) in PromisesAwaitingConnection) + foreach (TaskCompletionSource waiter in TakeWaiters()) { - reject(new OperationCanceledException("Connection was intentionally closed.")); + waiter.TrySetCanceled(); } - - PromisesAwaitingConnection = new List<(Action resolve, Action reject)>(); } + /// Waits until the connection is up, or until it is given up on. public async Task AwaitConnection() { - var tcs = new TaskCompletionSource(); - PromisesAwaitingConnection.Add((() => tcs.SetResult(null), (ex) => tcs.SetException(ex))); - await tcs.Task; + // RunContinuationsAsynchronously is not optional here - see the note on the class. + TaskCompletionSource waiter = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + lock (_waitersLock) + { + _promisesAwaitingConnection.Add(waiter); + } + + await waiter.Task; } } -} \ No newline at end of file +} diff --git a/Xrpl/Client/Exceptions/XrplException.cs b/Xrpl/Client/Exceptions/XrplException.cs index 51f274e2..1152edd7 100644 --- a/Xrpl/Client/Exceptions/XrplException.cs +++ b/Xrpl/Client/Exceptions/XrplException.cs @@ -89,7 +89,180 @@ public class NotConnectedException : XrplException public NotConnectedException(string message = null) : base(message ?? DefaultMessage) { } + + /// + /// Keeps the failure that caused this one. + /// + /// + /// A subtype that has a cause - the handler exception behind + /// - has nowhere else to put it: + /// has no setter, so it can only be passed up to + /// at construction. + /// + public NotConnectedException(string message, Exception? innerException) + : base(message ?? DefaultMessage, innerException) + { + } + } + /// + /// The client is not connected because the consumer disconnected it. + /// + /// + /// Nothing is going to bring the connection back on its own: Connect() is the only way + /// out. A consumer that reacts to a lost connection by failing over to another server must not + /// do so here - the client is down because it was asked to be. + /// + public class ClientDisconnectedException : NotConnectedException + { + public ClientDisconnectedException(string message = null) : base(message) { } } + + /// + /// The reconnect loop spent its budget of attempts and stopped. + /// + /// + /// This is the outcome that means the endpoint is not answering, and the one where failing + /// over to another server is the right reaction. It is reported only under + /// ConnectionOptions.StopAfterMaxAttempts; without it the loop keeps trying and there is + /// nothing to report. + /// + public class ReconnectExhaustedException : NotConnectedException + { + /// How many attempts were made. Equal to . + public int Attempts { get; } + + /// The budget that was configured, ConnectionOptions.MaxReconnectAttempts. + public int MaxAttempts { get; } + + public ReconnectExhaustedException(string message, int attempts, int maxAttempts) + : base(message) + { + Attempts = attempts; + MaxAttempts = maxAttempts; + } + } + + /// + /// The node closed the connection with a status code this client does not reconnect after, and + /// no reconnect was started. + /// + /// + /// + /// Distinct from , which says the consumer took the + /// client down, and from , which says this client + /// tried and ran out of budget. Here nothing was tried, because the close code said retrying + /// against this server is pointless - a protocol error, an unacceptable payload, a policy + /// violation. Reconnecting to the same endpoint is the one reaction that is certainly wrong; + /// a consumer with another server should use it, and one without should surface the close. + /// + /// + /// The status stream reports the same event as + /// ConnectionStopReason.ClosedPermanently. + /// + /// + public class ConnectionClosedPermanentlyException : NotConnectedException + { + public ConnectionClosedPermanentlyException(string message = null) : base(message) { } + } + + /// + /// The request was refused at once because the client was not connected and the policy in force + /// is RequestFailurePolicy.ImmediateFail. + /// + /// + /// This says nothing about the server: the connection was being rebuilt, and the caller asked + /// not to wait. Retrying once connected is the reaction; failing over is not. + /// + public class RequestRefusedException : NotConnectedException + { + public RequestRefusedException(string message = null) : base(message) { } + } + + /// + /// The client gave up because its own OnConnected handler kept failing. + /// + /// + /// The node answered and the socket opened; what failed is consumer code running on this side, + /// so moving to another server would be moving away from a healthy endpoint. The handler's own + /// failure is kept in . + /// + public class ConnectHandlerFailedException : NotConnectedException + { + /// How many times in a row the handler failed before the client gave up. + public int Failures { get; } + + public ConnectHandlerFailedException(string message, int failures, Exception? innerException = null) + : base(message, innerException) + { + Failures = failures; + } + } + + /// + /// There is no connection and no attempt to make one. + /// + /// + /// Distinct from every other outcome in this family: nothing failed, nothing gave up, and + /// nothing is in progress - Connect() was never called, or the client has settled after + /// stopping. Waiting would wait forever, which is why it is reported instead. + /// + public class NotConnectingException : NotConnectedException + { + public NotConnectingException(string message = null) : base(message) { } + } + + /// + /// The operation was overtaken by a later one, which now owns the connection. + /// + /// + /// + /// A transition of the connection has one owner. An operation that finds the connection has + /// moved on stands down and reports this instead of returning success from a server the client + /// is no longer on. The same is reported to a request that was in flight when a transition + /// swept it. + /// + /// + /// It derives from because that is what these paths + /// have always thrown, so no existing catch and no task status changes. Two consequences + /// are worth knowing: + /// + /// + /// + /// awaited directly, this exception - and therefore - reaches the caller. + /// Through Task.WhenAll it also reaches the caller, as long as none of the combined + /// tasks faulted. If one did, WhenAll records the faults only and drops the + /// cancellation: the supersession is then absent from the await and from + /// Task.Exception alike. Await the operation itself when the outcome matters. + /// + /// + /// is + /// : nobody cancelled anything, so a filter of the form + /// when (ex.CancellationToken == myToken) does not match here. + /// + /// + /// + public class ConnectionSupersededException : OperationCanceledException + { + /// What kind of operation took the connection over. + public ConnectionTransitionKind Kind { get; } + + /// + /// Where the operation that took over left the client, when that is known; otherwise + /// null. + /// + public string? SupersededBy { get; } + + public ConnectionSupersededException( + string message, + ConnectionTransitionKind kind, + string? supersededBy = null) + : base(message) + { + Kind = kind; + SupersededBy = supersededBy; + } + } + /// /// Exception thrown when xrpl.js has disconnected from rippled server. /// diff --git a/Xrpl/Client/IXrplClient.cs b/Xrpl/Client/IXrplClient.cs index 0b34a01b..237a7016 100644 --- a/Xrpl/Client/IXrplClient.cs +++ b/Xrpl/Client/IXrplClient.cs @@ -120,6 +120,19 @@ public interface IXrplClient : IDisposable /// long StaleSessionFramesDropped => connection.StaleSessionFramesDropped; + /// + /// + /// Defaulted, like : forwarding to + /// is the only implementation that means anything, and an external + /// implementer of this interface should not have to write one. A default member is reached + /// only through an interface-typed reference, so carries its own + /// as well. + /// + Task WaitForConnectionOutcomeAsync( + TimeSpan? timeout = null, + CancellationToken cancellationToken = default) => + connection.WaitForConnectionOutcomeAsync(timeout, cancellationToken); + /// /// How many stream frames were dispatched outside the queue, without its ordering or its /// capacity bound. @@ -743,6 +756,12 @@ public class ClientOptions : ConnectionOptions /// public long DroppedStreamMessages => connection.DroppedStreamMessages; + /// + public Task WaitForConnectionOutcomeAsync( + TimeSpan? timeout = null, + CancellationToken cancellationToken = default) => + connection.WaitForConnectionOutcomeAsync(timeout, cancellationToken); + /// public long StaleSessionFramesDropped => connection.StaleSessionFramesDropped; @@ -913,7 +932,13 @@ public async Task ChangeServer(string server, ClientOptions? options = null, Can SetSettings(options); await connection.ChangeServer(server, options, cancellationToken); - await SetNetworkId(); + + // Carried across a teardown, exactly as Connect() does it. A switch is the same two + // operations as a connection - the connection, and the server_info that reads the + // network id - and the connection can still be settling when the second one goes out. + // Read once and directly, it failed the whole switch on a connection that would have + // been up a moment later. + await SetNetworkIdWhileConnectingAsync(cancellationToken); } /// @@ -974,7 +999,7 @@ private async Task SetNetworkIdWhileConnectingAsync(CancellationToken cancellati return; } catch (Exception error) when ( - error is OperationCanceledException or DisconnectedException && + IsWorthAnotherNetworkIdAttempt(error) && !cancellationToken.IsCancellationRequested && attempt < attempts) { @@ -983,6 +1008,37 @@ error is OperationCanceledException or DisconnectedException && } } + /// + /// Whether a failed network-id read is worth another attempt. + /// + /// + /// + /// The line is drawn between the client's own lifecycle and a peer operation. A connection + /// being rebuilt is exactly what this loop exists for, and so is a teardown the client + /// performed on itself - giving up on a failing OnConnected handler ends by + /// disconnecting, and the request in flight dies with it. Asking again there is what lets + /// the wait inside the next attempt report the real reason the client stopped, instead of + /// the incidental sweep that happened to kill this request. Which of the two arrives is a + /// matter of timing, and the caller of Connect() should not be told a different + /// story depending on it. + /// + /// + /// A Connect() or ChangeServer from somewhere else is different: the + /// connection belongs to that operation now, asking again would read the network id of a + /// server this caller never asked for, and "your operation was overtaken" is the whole + /// answer. + /// + /// + private static bool IsWorthAnotherNetworkIdAttempt(Exception error) => + error switch + { + ConnectionSupersededException superseded => + superseded.Kind is not (ConnectionTransitionKind.Connect or ConnectionTransitionKind.ChangeServer), + OperationCanceledException => true, + DisconnectedException => true, + _ => false, + }; + private async Task SetNetworkId() { var server = await ServerInfo(new ServerInfoRequest()); diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index 8e17d03a..137e4ee7 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -51,6 +51,39 @@ public enum XrpConnectionState RestoringConnection, } +/// +/// The kind of operation that moved the connection. +/// +/// +/// +/// Carried by , which is how a +/// caller learns that its operation was overtaken - and by what. The reaction differs: another +/// put the client on a server the caller did not ask for, while +/// means the health check rebuilt the connection to the same one. +/// +/// +/// is reachable only on a request that was in flight when +/// Disconnect() swept it. An operation overtaken by a Disconnect() is told the +/// client is down - - which is +/// what that path has always reported and what a caller catching +/// still expects. +/// +/// +public enum ConnectionTransitionKind +{ + /// A Connect() from the consumer. + Connect, + + /// A ChangeServer from the consumer. + ChangeServer, + + /// A Disconnect() or DisconnectAndWaitAsync() from the consumer. + Disconnect, + + /// A reconnect the client started on its own, from the health check. + Reconnect, +} + public class ReconnectInfo { public int CurrentAttempt { get; set; } @@ -60,6 +93,90 @@ public class ReconnectInfo public TimeSpan RemainingDelay { get; set; } } +/// +/// Why the client stopped, on the notification that says it stopped. +/// +/// +/// +/// is announced from every ending a connection can +/// have, and they call for different reactions: a consumer's own disconnect is not a failure, a +/// spent reconnect budget is a reason to try another server, and a broken OnConnected +/// handler is a reason to fix the handler rather than move away from a node that is answering. +/// Until now they differed only in the text of the message. +/// +/// +/// It lives here rather than in on purpose: filling that in on a +/// terminal notification would give Reconnect != null a second meaning, when consumers +/// read it as "a reconnect is in progress". +/// +/// +public enum ConnectionStopReason +{ + /// The notification is not a terminal one. + None, + + /// The consumer called Disconnect() or DisconnectAndWaitAsync(). + UserDisconnected, + + /// The reconnect loop spent its budget of attempts and stopped. + ReconnectExhausted, + + /// The client gave up because its OnConnected handler kept failing. + ConnectHandlerFailed, + + /// The connection was closed for good, with no reconnect to follow. + ClosedPermanently, +} + +/// +/// How a wait for the connection ended. +/// +/// +/// +/// The same events and its subtypes +/// report, for callers who would rather read an answer than catch one: "it did not come back in +/// time" is something a caller has to act on, not an exceptional event. +/// +/// +/// A bool would fold "timed out", "gave up" and "nothing is running" into one false, +/// which is the confusion this whole family of types exists to remove. Two things stay exceptions: +/// cancellation through the caller's own token, which is the .NET convention, and an invalid +/// timeout, which is a mistake by the caller rather than an outcome of the connection. +/// +/// +public enum ConnectionWaitOutcome +{ + /// The connection is up. + Connected, + + /// It did not come up within the time allowed. + TimedOut, + + /// The reconnect loop spent its budget and stopped. + ReconnectExhausted, + + /// + /// The consumer disconnected the client. The counterpart of + /// , and named after it: the two enums are + /// two views of one event, and a pair that agrees on the event while disagreeing on its name + /// is a pair nothing can check. + /// + UserDisconnected, + + /// The client gave up because its OnConnected handler kept failing. + ConnectHandlerFailed, + + /// + /// The node closed the connection with a code this client does not reconnect after, and no + /// reconnect was started. The counterpart of + /// . + /// + ClosedPermanently, + + /// There is no connection and no attempt to make one: Connect() is due. + NotConnecting, +} + public class ConnectionStatusInfo { public string Message { get; set; } @@ -69,6 +186,9 @@ public class ConnectionStatusInfo public ReconnectInfo? Reconnect { get; set; } public XrpConnectionState ConnectionState { get; set; } + + /// + public ConnectionStopReason StopReason { get; set; } } public class Connection @@ -363,6 +483,72 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con private int _reconnectAttempts = 0; + /// + /// The generation whose reconnect sequence spent its budget and stopped, or 0. + /// + /// + /// + /// StopAfterMaxAttempts is a promise that the client stops asking, and until this was + /// recorded nothing kept it. A sequence that gives up clears the two fields + /// reads to decide whether one is already running - + /// and - which is exactly + /// what "none is running" looks like. The close of the attempt that failed last arrives after + /// that and was indistinguishable from the close that started the whole thing, so a second + /// full series ran behind a state that had already reported the client stopped, with the + /// attempt counter back at zero. + /// + /// + /// Keyed by generation rather than flagged, so it needs no clearing: generations only ever + /// increase, and a Connect() or ChangeServer begins a new one - which is + /// precisely when asking again is the consumer's decision and allowed. + /// + /// + private long _reconnectExhaustedGeneration = NoGeneration; + + /// + /// Not any generation. Generations are counted from zero and the first transition raises the + /// counter to one, so zero is the generation of a client nobody has called Connect() + /// on yet - a real value, and the wrong one to spell "no generation" with: every field below + /// that used zero for it answered its own question with "yes" on a brand-new client. + /// + private const long NoGeneration = -1; + + /// + /// The generation that was last announced as stopped, or , together + /// with the reason it was announced with. Written by for + /// every terminal status it actually publishes; read by the wait and by + /// . + /// + /// + /// + /// One record rather than a field per ending, and that is the point of it. Each ending used to + /// leave its own residue for a caller to recognise - a flag for the consumer's own disconnect, + /// a generation for a spent reconnect budget - and an ending that left none was invisible to + /// everyone except the status stream. A caller already parked was woken by the announcement, + /// found nothing that reads as an ending, and parked again on a signal nothing was going to + /// complete: the whole acquisition timeout, five minutes by default, spent to be told the + /// connection "was not established in time" about a connection the consumer had already been + /// told was over. Two endings were found that way, in three places, and the third would have + /// been found later. + /// + /// + /// Recording it where the announcement happens is what makes the set closed: a terminal reason + /// cannot reach a consumer without passing through the one method that writes this, so the + /// status stream and the wait cannot come to know different things. Keyed by generation for + /// the same reason as , so nothing has to clear it. + /// + /// + /// stays beside it and is not folded in: it has a + /// second job - refuses a generation equal to it - and it is + /// deliberately written *before* the announcement, so that a close arriving while the + /// notification runs consumer code already finds the sequence marked over. + /// + /// + private long _stoppedGeneration = NoGeneration; + + /// + private ConnectionStopReason _stoppedReason = ConnectionStopReason.None; + // Number of consecutive times the consumer OnConnected handler threw. // Not part of the reconnect state: OnceOpen clears the reconnect state before invoking the handler, // so this counter is the only thing that can bound an endlessly failing handler. @@ -465,6 +651,168 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con private volatile bool _permanentlyDisconnected = false; + /// + /// What a caller waiting for the connection sleeps on. + /// + /// + /// + /// Completed when the connection comes up, and when the client reaches a state it will not come + /// back from - a disconnect, or a reconnect loop that spent its budget. A waiter re-reads the + /// state after every completion, so the signal only has to say "look again"; the answer itself + /// is still decided by the same checks, which is what keeps the wait's behaviour identical to + /// the polling loop it replaces. + /// + /// + /// A retirement deliberately does NOT complete it. The connection is being rebuilt, and a + /// caller waiting for it - a request under + /// , above all - has to carry over to the + /// new connection rather than be told the old one went away. Waking them there would turn a + /// documented carry-over into a failure. + /// + /// + /// Completed with a value rather than an exception: nobody may be waiting, and a faulted task + /// with no observer is an unobserved exception. The reason is built at the throw site, where it + /// already was. + /// + /// + private TaskCompletionSource _connectionReady = NewReadySignal(); + + /// + /// RunContinuationsAsynchronously is not optional: the signal is completed from inside + /// the critical sections that move the connection, and without it every parked waiter would + /// resume inline there - the same defect as issue #177, in a new place. + /// + /// The longest single wait accepts. + private static readonly TimeSpan MaxSignalWait = TimeSpan.FromMilliseconds(int.MaxValue); + + private static TaskCompletionSource NewReadySignal() => + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + /// + /// Wakes everyone waiting for the connection, so they re-read the state, and arms a fresh + /// signal for the next change in the same breath. + /// + /// + /// Re-arming here rather than where an attempt begins is what makes the wait safe: a + /// connection can also go away through its socket's close callback, which continues the + /// generation and takes over nothing. Arming only on takeover left the signal standing + /// completed after such a close, and a waiter then span on it - awake, re-reading a state that + /// said "not connected", until its own timeout. Since a completed signal is replaced in the + /// same critical section that completes it, a waiter either holds one that is about to be + /// completed or takes the next one on its following pass. + /// + private void WakeConnectionWaiters() + { + TaskCompletionSource waking; + lock (_transitionLock) + { + waking = _connectionReady; + _connectionReady = NewReadySignal(); + } + + waking.TrySetResult(true); + } + + /// + /// Why the client is permanently disconnected, when the reason is not "the consumer asked". + /// + /// + /// Set by the path that gives up on a failing OnConnected handler, just before it + /// announces the ending - the announcement is the first thing anyone can see that ending by, + /// and a caller reading the cause a moment later must not be told the consumer disconnected + /// the client. The disconnect that path performs is handed the same value, and + /// replaces or clears it under in + /// the same statement group as , so a takeover by + /// anything else ends its lifetime. + /// + private ConnectHandlerFailure? _connectHandlerGaveUp; + + /// + /// What a failing OnConnected handler cost, kept so that every point reporting the + /// resulting disconnect can say what really happened rather than "the client was disconnected". + /// + private sealed record ConnectHandlerFailure(string Message, int Failures, Exception? Error); + + /// + /// The exception for "the client is not connected because it was taken down", with the cause + /// filled in. + /// + /// + /// A consumer's Disconnect() and the client giving up on a broken handler both leave the + /// same state, and both used to be reported as the same bare exception. They call for opposite + /// reactions - do nothing versus fix the handler - so they are told apart here, once, instead + /// of at each of the three points that report it. + /// + private NotConnectedException DisconnectedBecause(string disconnectedMessage) + { + lock (_transitionLock) + { + return DisconnectedBecauseLocked(disconnectedMessage); + } + } + + /// + /// Must be called with held. + private NotConnectedException DisconnectedBecauseLocked(string disconnectedMessage) + { + ConnectHandlerFailure? gaveUp = _connectHandlerGaveUp; + + return gaveUp is null + ? new ClientDisconnectedException(disconnectedMessage) + : new ConnectHandlerFailedException(gaveUp.Message, gaveUp.Failures, gaveUp.Error); + } + + /// + /// The exception for the ending this generation was announced with, or null when it was + /// not announced as stopped. + /// + /// + /// + /// The one place that turns a into the type a caller gets, + /// so that every way of asking - the wait, its outcome value, and the check a request makes - + /// answers from the same record and cannot drift apart. Every reason other than + /// is mapped; a reason added without a case here is a + /// compile-time hole only in the sense that the switch returns null, which is why the mapping + /// is asserted by a test that enumerates the enum. + /// + /// + /// and + /// both go through + /// , which is what the permanently-disconnected flag + /// answers with too, so a caller gets the same type whichever of the two records it is read + /// from. The give-up path announces before it calls Disconnect(), so in the moment + /// between them the cause is not yet recorded and this answers + /// ClientDisconnectedException; it is the right family and a terminal answer, which is + /// what the caller is owed - it used to be nothing at all. + /// + /// Must be called with held. + /// + private NotConnectedException? StoppedBecauseLocked() + { + if (_stoppedGeneration != _generation) + { + return null; + } + + return _stoppedReason switch + { + ConnectionStopReason.UserDisconnected or ConnectionStopReason.ConnectHandlerFailed => + DisconnectedBecauseLocked("Client has been disconnected. Call Connect() to reconnect."), + + ConnectionStopReason.ReconnectExhausted => new ReconnectExhaustedException( + $"Connection failed permanently after {config.MaxReconnectAttempts} attempts. " + + "Reconnection has been stopped.", + attempts: config.MaxReconnectAttempts, + maxAttempts: config.MaxReconnectAttempts), + + ConnectionStopReason.ClosedPermanently => new ConnectionClosedPermanentlyException( + "The node closed the connection with a code this client does not reconnect after. " + + "Call Connect() to try again, or connect to another server."), + + _ => null, + }; + } + private volatile bool _isIntentionalDisconnect = false; // Socket that was closed due to ping timeout - late callbacks from this socket should be ignored @@ -766,12 +1114,22 @@ private bool TryTakeOverFrom( private Takeover TakeOverLocked( TransitionKind kind, bool retireSession, - out CancellationTokenSource? retiredCts) + out CancellationTokenSource? retiredCts, + ConnectHandlerFailure? handlerFailure = null) { long generation = ++_generation; _generationKind = kind; _permanentlyDisconnected = kind == TransitionKind.Disconnect; + // Written here, in the same statement group as the flag it qualifies, so the two cannot + // drift: the cause lives exactly as long as the disconnect it describes and is cleared by + // the same takeover that clears the flag. The path that gives up on a failing OnConnected + // handler ends by calling Disconnect() itself, and without this every point that reads + // _permanentlyDisconnected answered "the consumer disconnected the client" for a client + // that is down because its own handler is broken - the opposite reaction for a consumer + // deciding whether to fail over to another server. + _connectHandlerGaveUp = _permanentlyDisconnected ? handlerFailure : null; + // 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 @@ -879,39 +1237,198 @@ private void ThrowIfSupersededLocked(long generation) /// 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. /// + /// + /// The private transition kind as a consumer sees it. + /// + /// + /// None means no transition is in progress, which cannot be the winner of one; a client + /// that reports it here has been overtaken by something that has already finished, and + /// is the honest answer - the client rebuilt + /// its own connection. + /// + private ConnectionTransitionKind PublicKindLocked() => + _generationKind switch + { + TransitionKind.Connect => ConnectionTransitionKind.Connect, + TransitionKind.ChangeServer => ConnectionTransitionKind.ChangeServer, + TransitionKind.Disconnect => ConnectionTransitionKind.Disconnect, + _ => ConnectionTransitionKind.Reconnect, + }; + + /// + /// The exception a request in flight gets when a transition takes the connection away from it. + /// + /// + /// + /// Every such sweep used to reject with a bare reading + /// "Connection was intentionally closed", which a caller could not tell from a cancellation of + /// its own. It is the failure consumers meet most often - their request dying while the + /// connection moved underneath it - and it said nothing about what moved it or where. + /// + /// + /// Still an : a caller that treated a swept request as + /// cancellation keeps working, and the task keeps the status it had. Sweeps caused by the + /// connection failing on its own - a network drop, a close being processed - deliberately keep + /// the plain cancellation; they are not a transition and have no destination to name, and the + /// choice of cancellation there is what keeps consuming applications from logging an ordinary + /// network drop as a critical error. + /// + /// + private static ConnectionSupersededException SweptBy(ConnectionTransitionKind kind, string? destination) => + new ConnectionSupersededException( + kind switch + { + ConnectionTransitionKind.ChangeServer => + $"The request was dropped: the client switched to {destination}.", + ConnectionTransitionKind.Connect => + "The request was dropped: a Connect() rebuilt the connection.", + ConnectionTransitionKind.Disconnect => + "The request was dropped: the client was disconnected.", + _ => + "The request was dropped: the connection was rebuilt by the health check.", + }, + kind, + destination); + 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."), + TransitionKind.Disconnect => DisconnectedBecauseLocked("Client has been disconnected. Call Connect() to reconnect."), + TransitionKind.ChangeServer => new ConnectionSupersededException( + $"Superseded by a later ChangeServer to {url}.", + ConnectionTransitionKind.ChangeServer, + supersededBy: url), + TransitionKind.Connect => new ConnectionSupersededException( + "Superseded by a later Connect().", + ConnectionTransitionKind.Connect, + supersededBy: url), + _ => new ConnectionSupersededException( + "Superseded by a reconnect the health check started.", + ConnectionTransitionKind.Reconnect, + supersededBy: url), }; public XrpConnectionState CurrentConnectionState => _currentConnectionState; private string _previousNotifiedMessage = string.Empty; + private ConnectionStopReason _previouslyNotifiedStopReason = ConnectionStopReason.None; + + /// + /// The transition this status speaks for. Required rather than optional, and that is the whole + /// of its design: an announcement is true only while the transition it belongs to still owns + /// the connection, and every time this was left to the call sites to remember, a call site + /// forgot. Five separate reviews found five such sites on this branch alone. Passing it is now + /// the only way to compile, so the next one cannot be forgotten - it can only be wrong, which + /// is a smaller and more visible mistake. + /// private void SetConnectionState( XrpConnectionState newState, string message, + long announcingGeneration, ConnectionCloseSeverity severity = ConnectionCloseSeverity.Info, - ReconnectInfo? reconnect = null) + ReconnectInfo? reconnect = null, + ConnectionStopReason stopReason = ConnectionStopReason.None) { + bool stateChanged = false; + bool suppressed = false; + + // The two questions this guard asks and the publication it guards are one critical + // section, and that is the whole of the point. Who owns the connection, and whether this + // generation's reconnect sequence has already been declared over, are both written under + // this lock by other threads. Asked outside it, an answer is a snapshot with no shelf + // life: a caller reads "the sequence is still running", loses the processor, and publishes + // its status after the loop has ended the sequence and announced the ending - which is a + // non-terminal status standing as the last word about a client nothing is rebuilding. + // + // It replaces the ownership checks that used to sit at the call sites. Three of them + // existed, one was missing, and the missing one was invisible for as long as the + // deduplication happened to swallow what it let through. + // + // A terminal announcement is never suppressed by the seal - it is the seal - and callers + // that pass no generation speak for the client as a whole rather than for one transition + // of it, and are not guarded at all. + lock (_transitionLock) + { + // The seal stops a spent sequence reporting progress it is not making. A connection + // that actually came up is not progress-chatter but the fact that contradicts the + // seal, and suppressing it would leave the record standing over a client that is up - + // every caller told the sequence gave up while the socket carries traffic. So + // Connected passes the seal, and only the ownership check applies to it. + bool sealedOff = _reconnectExhaustedGeneration == announcingGeneration && + stopReason == ConnectionStopReason.None && + newState != XrpConnectionState.Connected; - var stateChanged = _currentConnectionState != newState; - _currentConnectionState = newState; + if (_generation != announcingGeneration || sealedOff) + { + suppressed = true; + } + else + { + stateChanged = _currentConnectionState != newState; + _currentConnectionState = newState; + + // Every ending a consumer is told about is recorded here, in the same critical + // section that publishes it, so that the wait cannot know less than the status + // stream does. Suppressed announcements are not recorded: an announcement nobody + // is shown did not happen. + if (stopReason != ConnectionStopReason.None) + { + _stoppedGeneration = announcingGeneration; + _stoppedReason = stopReason; + } + else if (newState == XrpConnectionState.Connected) + { + // An ending is over when the connection is up again, and this is the + // counterpart of recording it. Without it a generation announced as stopped + // that connected anyway - a straggling attempt succeeding after its loop gave + // up - would keep answering every caller with the ending, because the record + // outlives the state it describes and nothing else clears it. Connected is the + // one state that contradicts "stopped"; RestoringConnection does not, which is + // why only this one clears. + _stoppedGeneration = NoGeneration; + _stoppedReason = ConnectionStopReason.None; + } + } + } + + // Woken here, before everything else this method decides. Whether a status event is worth + // showing a consumer and whether the connection changed are different questions: the + // deduplication below drops a notification that repeats the last one, and a waiter parked + // across such a change - a disconnect reported on a client already reported as + // disconnected - would never hear about it. Waking also precedes the consumer callback, + // which is code this class does not control and already has to be guarded against: a slow + // handler must not hold up a caller waiting for the connection, and one that waits on such + // a caller must not be able to deadlock against it. + WakeConnectionWaiters(); + + // After the wake, deliberately. A suppressed announcement still means something happened + // to the connection, and the wait's own predicate reads the same fields under the same + // lock: it may have a terminal answer waiting for it even when this particular status is + // not worth showing anyone. + if (suppressed) + { + return; + } var hasReconnectInfo = reconnect != null; var messageChanged = _previousNotifiedMessage != message; var isRestoringConnection = newState == XrpConnectionState.RestoringConnection; - if (!stateChanged && !hasReconnectInfo && !(isRestoringConnection && messageChanged)) + // The reason counts as a change in its own right. Two endings in a row are both + // Disconnected - an initial connection that never came up, then the consumer disconnecting + // - and without this the second is dropped as a repeat, leaving the consumer reading a + // reason that belongs to the ending before it. A field nobody can rely on being emitted is + // worse than no field. + var reasonChanged = _previouslyNotifiedStopReason != stopReason; + + if (!stateChanged && !hasReconnectInfo && !reasonChanged && !(isRestoringConnection && messageChanged)) { return; } _previousNotifiedMessage = message; + _previouslyNotifiedStopReason = stopReason; // Contained here, once, rather than at each call site. Every state notification in this class // funnels through this method, and several call sites are places where an escaping exception @@ -928,6 +1445,7 @@ private void SetConnectionState( Severity = severity, Reconnect = reconnect, ConnectionState = newState, + StopReason = stopReason, }); } catch (Exception notifyError) @@ -1092,14 +1610,18 @@ public async Task ChangeServer( { // 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}..."); + SetConnectionState( + XrpConnectionState.Connecting, + message: $"ChangeServer: Switching to {server}...", + announcingGeneration: generation); ThrowIfSuperseded(generation); // 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(); + ConnectionSupersededException switched = SweptBy(ConnectionTransitionKind.ChangeServer, server); + requestManager.RejectAll(switched); + connectionManager.RejectAllAwaiting(switched); ThrowIfSuperseded(generation); // The message processor went with the session, and its reader is let go of after the @@ -1147,11 +1669,24 @@ public async Task ChangeServer( // 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(); + // Where the client ended up and what put it there are read together, under the lock that + // publishes both. Read apart, another takeover between the two gives an exception naming + // one transition's destination and another transition's kind - a description of a client + // state that never existed. + string connectedTo; + ConnectionTransitionKind winner; + lock (_transitionLock) + { + connectedTo = url; + winner = PublicKindLocked(); + } + if (!string.Equals(connectedTo, server, StringComparison.Ordinal)) { - throw new OperationCanceledException( - $"ChangeServer to {server} was superseded by a later ChangeServer to {connectedTo}."); + throw new ConnectionSupersededException( + $"ChangeServer to {server} was superseded by a later ChangeServer to {connectedTo}.", + winner, + supersededBy: connectedTo); } } @@ -1227,8 +1762,9 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason, WebSocke SetConnectionState( XrpConnectionState.RestoringConnection, message: $"{reason} Reconnecting immediately...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); // 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 @@ -1242,8 +1778,9 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason, WebSocke // 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(); + ConnectionSupersededException rebuilding = SweptBy(ConnectionTransitionKind.Reconnect, url); + requestManager.RejectAll(rebuilding); + connectionManager.RejectAllAwaiting(rebuilding); if (!Owns(generation)) { @@ -1351,8 +1888,9 @@ await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ConnectionLost, SetConnectionState( XrpConnectionState.RestoringConnection, message: $"Reconnection failed: {ex.Message}. Retrying...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); } } @@ -1360,13 +1898,11 @@ await NotifySessionEndedAsync(takeover.Session, SessionEndReason.ConnectionLost, public async Task WaitForConnectionAsync(TimeSpan? timeout = null, CancellationToken cancellationToken = default) { - if (IsConnected()) - { - return; - } - - CheckIfNotConnected(); - + // No fast path on IsConnected() here. The loop below answers the same question one lock + // later and answers it with the rest of the state in hand, which is the whole difference: + // a socket can be open while the client has already been announced as stopped, and a + // caller that reads only the socket is told the connection is up moments after it was told + // why it is over. var waitTimeout = timeout ?? config.ConnectionAcquisitionTimeout; if (waitTimeout != Timeout.InfiniteTimeSpan && waitTimeout <= TimeSpan.Zero) @@ -1378,30 +1914,130 @@ public async Task WaitForConnectionAsync(TimeSpan? timeout = null, CancellationT } var startTime = DateTime.UtcNow; - var checkInterval = TimeSpan.FromMilliseconds(100); var hasTimeout = waitTimeout != Timeout.InfiniteTimeSpan; - while (!IsConnected()) - { - // Re-checked on every iteration, not only on entry: the client can be disconnected while a - // caller is already waiting here (user Disconnect(), or the client giving up on a permanently - // failing OnConnected handler). Without this the caller would sit out the whole acquisition - // timeout and get a generic TimeoutException instead of the actual reason. - if (_permanentlyDisconnected) + // "Nothing is in progress" is a statement about the moment of the call and nothing else, + // which is why it is asked once. Asked on every pass it would fire on a caller that is + // waiting perfectly correctly: a failed first attempt announces Disconnected and only then + // starts the reconnect loop, and in between there is no socket, no loop and no active + // state - exactly the reading this answers with "call Connect()". + bool firstPass = true; + + while (true) + { + // The signal and everything the decision rests on are read in one critical section, + // under the lock every transition publishes through. Two properties come from that, + // and the wait is wrong without either. + // + // Order: the signal is captured before the state is read. Captured after, it would be + // the replacement armed by a change that landed in between - so a waiter would park on + // a signal for a change that had already happened, and sit out its timeout with the + // answer in front of it. + // + // Consistency: these fields are written under this lock by transitions that change + // several of them at once. Read outside it they can be a mixture from two transitions, + // and - since this wait no longer polls - a decision made on such a mixture is not + // corrected a tick later but stands until the timeout. + Task ready; + bool connected; + NotConnectedException? terminal = null; + lock (_transitionLock) { - throw new NotConnectedException("Client has been disconnected. Call Connect() to reconnect."); + ready = _connectionReady.Task; + connected = IsConnected(); + + // These two are asked whether or not a socket happens to be open, and that is the + // opposite of the order this used to have. The give-up path rejects the requests + // in flight - which is what resumes the caller inside Connect() - one statement + // before it disconnects, so for that moment the socket is still installed and + // still open while the consumer has already been told the client gave up. A caller + // asking then was told it was connected: a failure reporting itself as success, + // and the two ways of asking contradicting each other about one event. The record + // read here is written by the announcement that precedes the rejection, so the + // ordering is guaranteed rather than raced. + // + // Re-checked on every pass, not only on entry: the client can be disconnected + // while a caller is already waiting here - a user Disconnect(), or the client + // giving up on a permanently failing OnConnected handler. + if (_permanentlyDisconnected) + { + terminal = DisconnectedBecauseLocked( + "Client has been disconnected. Call Connect() to reconnect."); + } + // Whatever this generation was announced as stopped with. Asked after the + // permanently-disconnected flag because that one is set by the takeover rather + // than by an announcement and is therefore true earlier, and before the reconnect + // budget below because that one answers on its own residue. + else if (StoppedBecauseLocked() is NotConnectedException announced) + { + terminal = announced; + } + + if (terminal == null && !connected) + { + // The generation is asked first, and that ordering is what stops a waiter + // depending on work that has not happened yet. The loop records the generation + // as spent before it announces the fact, and releases its cancellation source + // only after the announcement returns - and the announcement runs consumer + // code. A status handler that blocks on a waiter would otherwise hold the loop + // on that notification while the waiter waited for a release the loop could no + // longer reach: the handler waits for the waiter, the waiter for the + // bookkeeping, the bookkeeping for the handler. Reading what is already + // published breaks the ring. The second condition stays for the same state + // reached without a loop of this generation having run. + if (_reconnectExhaustedGeneration == _generation || + (config.StopAfterMaxAttempts && + _reconnectAttempts >= config.MaxReconnectAttempts && + _reconnectCts == null)) + { + // Attempts is reported as the budget, not as the raw counter: the loop + // increments at the head of a pass and stops on the pass that exceeds the + // budget, so the counter stands one past it here and a consumer would read + // "6 of 5". + terminal = new ReconnectExhaustedException( + $"Connection failed permanently after {config.MaxReconnectAttempts} attempts. " + + "Reconnection has been stopped.", + attempts: config.MaxReconnectAttempts, + maxAttempts: config.MaxReconnectAttempts); + } + // Last of the four, and that order is the whole of what it is for. After a + // sequence ends there is no socket, no cancellation source and a Disconnected + // state - which is also precisely what a client nobody has called Connect() on + // looks like. Asked first, as the entry precondition it used to be, it + // answered "call Connect()" to a caller whose client had just spent its + // reconnect budget: the status stream said ReconnectExhausted, the wait said + // NotConnecting, and the outcome this API exists to deliver was reachable only + // by a caller who happened to be parked here already when the loop gave up. + else if (firstPass && + ws == null && + _reconnectCts == null && + _currentConnectionState == XrpConnectionState.Disconnected) + { + terminal = new NotConnectingException("No connection attempt in progress. Call Connect() first."); + } + } } - if (config.StopAfterMaxAttempts && - _reconnectAttempts >= config.MaxReconnectAttempts && - _reconnectCts == null) + firstPass = false; + + if (terminal != null) + { + throw terminal; + } + + if (connected) { - throw new NotConnectedException( - $"Connection failed permanently after {config.MaxReconnectAttempts} attempts. " + - "Reconnection has been stopped."); + return; } - if (hasTimeout && DateTime.UtcNow - startTime > waitTimeout) + // Inclusive. Both readings of the clock are quantised to the system tick - 15.625 ms + // on Windows - so elapsed time is always a whole number of ticks, and any timeout that + // is itself a whole number of them is hit exactly rather than passed: the default + // acquisition timeout of five minutes is 19200 of them. With a strict comparison the + // deadline check then declined to fire while the remaining time was already zero, and + // the guard below sent the loop round again with no await in it - a spin on the + // connection's own lock for the rest of the tick, once per expiring wait. + if (hasTimeout && DateTime.UtcNow - startTime >= waitTimeout) { throw new System.TimeoutException( $"Connection was not established within {waitTimeout.TotalSeconds:F1} seconds"); @@ -1412,9 +2048,35 @@ public async Task WaitForConnectionAsync(TimeSpan? timeout = null, CancellationT throw new OperationCanceledException(message: "Connection wait was cancelled", cancellationToken); } + TimeSpan remaining = hasTimeout + ? waitTimeout - (DateTime.UtcNow - startTime) + : Timeout.InfiniteTimeSpan; + + // Unreachable now that the deadline check above is inclusive, and kept because + // "the remaining time is not positive" must never again become "go round again + // without awaiting anything". + if (hasTimeout && remaining <= TimeSpan.Zero) + { + throw new System.TimeoutException( + $"Connection was not established within {waitTimeout.TotalSeconds:F1} seconds"); + } + + // Task.WaitAsync refuses any timeout above int.MaxValue milliseconds - about + // twenty-five days - and a caller may legitimately ask to wait longer than that; the + // validation above only refuses zero and negative. The polling loop this replaced had + // no such ceiling, so passing the remaining time straight through turned a long wait + // into an immediate ArgumentOutOfRangeException. The deadline is re-read at the head of + // every pass, so a long wait is simply served by several bounded ones. + TimeSpan waitSlice = hasTimeout && remaining > MaxSignalWait ? MaxSignalWait : remaining; + try { - await Task.Delay(checkInterval, cancellationToken); + await ready.WaitAsync(waitSlice, cancellationToken); + } + catch (System.TimeoutException) + { + // The wait's own deadline, re-reported by the check at the head of the next pass + // with the message this method has always used. } catch (OperationCanceledException) { @@ -1423,6 +2085,58 @@ public async Task WaitForConnectionAsync(TimeSpan? timeout = null, CancellationT } } + /// + /// + /// Waits for the connection and reports how the wait ended. + /// + /// + /// Each value maps to exactly one of the exceptions + /// throws, so the two ways of asking cannot drift apart: + /// to + /// , + /// to + /// , + /// to + /// , + /// to + /// , and + /// to . + /// + public async Task WaitForConnectionOutcomeAsync( + TimeSpan? timeout = null, + CancellationToken cancellationToken = default) + { + try + { + await WaitForConnectionAsync(timeout, cancellationToken); + return ConnectionWaitOutcome.Connected; + } + catch (ConnectHandlerFailedException) + { + return ConnectionWaitOutcome.ConnectHandlerFailed; + } + catch (ClientDisconnectedException) + { + return ConnectionWaitOutcome.UserDisconnected; + } + catch (ReconnectExhaustedException) + { + return ConnectionWaitOutcome.ReconnectExhausted; + } + catch (ConnectionClosedPermanentlyException) + { + return ConnectionWaitOutcome.ClosedPermanently; + } + catch (NotConnectingException) + { + return ConnectionWaitOutcome.NotConnecting; + } + catch (System.TimeoutException) + { + return ConnectionWaitOutcome.TimedOut; + } + } + public async Task HasConnectionAsync(TimeSpan? timeout = null) { try @@ -1444,9 +2158,34 @@ public async Task HasConnectionAsync(TimeSpan? timeout = null) public async Task Connect(CancellationToken cancellationToken) { - if (IsConnected()) + // An open socket alone does not say the client is up, and this is the same question the + // wait asks, answered the same way so the two cannot disagree. The give-up path for a + // failing OnConnected handler announces the ending, resumes the caller it had waiting, and + // disconnects two statements later; a consumer reacting to that announcement with + // Connect() - the obvious reaction, and the one the message asks for - found the socket + // still open and was told it was already connected, about a connection being torn down as + // it read the answer. Worse than a wrong status: Connect() returned success and started + // nothing, so nothing was going to reconnect. + bool alreadyConnected; + long connectedGeneration; + lock (_transitionLock) + { + alreadyConnected = IsConnected() && + !_permanentlyDisconnected && + StoppedBecauseLocked() == null; + connectedGeneration = _generation; + } + + if (alreadyConnected) { - SetConnectionState(XrpConnectionState.Connected, message: $"Already connected to {url}"); + // The generation is captured with the answer, not read at the announcement. A takeover + // landing in between makes this publication somebody else's business: untagged, it + // would put Connected over the state of the transition that won and clear a record + // that is not this call's to clear. + SetConnectionState( + XrpConnectionState.Connected, + message: $"Already connected to {url}", + announcingGeneration: connectedGeneration); return; } @@ -1466,7 +2205,7 @@ public async Task Connect(CancellationToken cancellationToken) // 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(); + requestManager.RejectAll(SweptBy(ConnectionTransitionKind.Connect, url)); // 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 @@ -1474,7 +2213,10 @@ public async Task Connect(CancellationToken cancellationToken) await takeover.ProcessorExit; Interlocked.Exchange(ref _connectHandlerFailures, value: 0); - SetConnectionState(XrpConnectionState.Connecting, message: $"Connecting to {url}..."); + SetConnectionState( + XrpConnectionState.Connecting, + message: $"Connecting to {url}...", + announcingGeneration: takeover.Generation); // 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 @@ -1728,7 +2470,8 @@ await errorHandler.Invoke( /// 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() + private (Takeover Takeover, TaskCompletionSource? Tcs) TakeOverForDisconnect( + ConnectHandlerFailure? handlerFailure = null) { // 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 @@ -1739,7 +2482,7 @@ await errorHandler.Invoke( CancellationTokenSource? retiredCts; lock (_transitionLock) { - takeover = TakeOverLocked(TransitionKind.Disconnect, retireSession: false, out retiredCts); + takeover = TakeOverLocked(TransitionKind.Disconnect, retireSession: false, out retiredCts, handlerFailure); WebSocketClient? socketToClose = takeover.Socket; if (socketToClose != null) @@ -1768,17 +2511,28 @@ await errorHandler.Invoke( return (takeover, tcs); } - public async Task Disconnect() + public async Task Disconnect() => await DisconnectAsync(handlerFailure: null); + + private async Task DisconnectAsync(ConnectHandlerFailure? handlerFailure) { - (Takeover takeover, _) = TakeOverForDisconnect(); + // The disconnect this path performs is not always the consumer's. Giving up on a broken + // OnConnected handler ends here too, and hard-coding "the user disconnected" made the last + // thing the status stream said contradict the exception the same event produced - the one + // contradiction this whole change exists to remove. + ConnectionStopReason stopReason = handlerFailure is null + ? ConnectionStopReason.UserDisconnected + : ConnectionStopReason.ConnectHandlerFailed; + + (Takeover takeover, _) = TakeOverForDisconnect(handlerFailure); long generation = takeover.Generation; WebSocketClient? socketToClose = takeover.Socket; // 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(); + ConnectionSupersededException takenDown = SweptBy(ConnectionTransitionKind.Disconnect, destination: null); + requestManager.RejectAll(takenDown); + connectionManager.RejectAllAwaiting(takenDown); await takeover.ProcessorExit; await WaitForPingToFinishAsync(); @@ -1789,7 +2543,11 @@ public async Task Disconnect() // ChangeServer that took over during the awaits above is reporting its own state now. if (Owns(generation)) { - SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + SetConnectionState( + XrpConnectionState.Disconnected, + message: "Already disconnected.", + stopReason: stopReason, + announcingGeneration: generation); } return 0; @@ -1800,7 +2558,11 @@ public async Task Disconnect() if (Owns(generation)) { - SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + SetConnectionState( + XrpConnectionState.Disconnected, + message: "Disconnected by user request.", + stopReason: stopReason, + announcingGeneration: generation); } // Announced here as well as from the socket's close callback, which dedups. The callback @@ -1828,8 +2590,9 @@ public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken can WebSocketClient? socketToClose = takeover.Socket; // Same ordering as Disconnect(): the socket left ws before the sweep runs (issue #177). - requestManager.RejectAllWithCancellation(); - connectionManager.RejectAllAwaitingWithCancellation(); + ConnectionSupersededException closing = SweptBy(ConnectionTransitionKind.Disconnect, destination: null); + requestManager.RejectAll(closing); + connectionManager.RejectAllAwaiting(closing); await takeover.ProcessorExit; await WaitForPingToFinishAsync(); @@ -1853,7 +2616,11 @@ public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken can if (Owns(generation)) { - SetConnectionState(XrpConnectionState.Disconnected, message: "Already disconnected."); + SetConnectionState( + XrpConnectionState.Disconnected, + message: "Already disconnected.", + stopReason: ConnectionStopReason.UserDisconnected, + announcingGeneration: generation); } return; @@ -1863,7 +2630,11 @@ public async Task DisconnectAndWaitAsync(TimeSpan timeout, CancellationToken can if (Owns(generation)) { - SetConnectionState(XrpConnectionState.Disconnected, message: "Disconnected by user request."); + SetConnectionState( + XrpConnectionState.Disconnected, + message: "Disconnected by user request.", + stopReason: ConnectionStopReason.UserDisconnected, + announcingGeneration: generation); } // See Disconnect() for why this is announced here and not left to the close callback. @@ -2285,18 +3056,30 @@ private async Task OnConnectionFailed( CompleteDisconnectTcs(); + // Read before the branch below, because that branch speaks about the connection too. 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 (intentionalDisconnect) { connectionManager.RejectAllAwaitingWithCancellation(); - SetConnectionState(XrpConnectionState.Disconnected, message: "Connection closed permanently."); + + // Announced only while this callback still speaks for the connection. The attempt + // timer of a handshake outlives the Disconnect() that cancelled it, and this branch + // then reported "closed permanently" over the disconnect the consumer had already been + // told about - the last reason they saw being one that does not say who closed it. + SetConnectionState( + XrpConnectionState.Disconnected, + message: "Connection closed permanently.", + stopReason: ConnectionStopReason.ClosedPermanently, + announcingGeneration: generation); + return; } // 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(); + // loop - and that belongs to whoever owns it. if (!Owns(generation)) { return; @@ -2315,13 +3098,36 @@ private async Task OnConnectionFailed( connectionManager.RejectAllAwaiting(new NotConnectedException(error.Message)); } + // The tail of a sequence that is over. This generation's reconnect loop spent its budget, + // announced it and stopped; the attempt that failed last reports here afterwards, and + // everything below would then speak for a connection nobody is rebuilding - a + // RestoringConnection carrying a reconnect block, or a Disconnected whose reason says the + // notification is not an ending, either of them landing after the terminal one a consumer + // was meant to act on. The StartReconnectLoop at the end is refused for the same reason, + // so the announcement would describe work that is not going to happen. + bool sequenceIsOver; + lock (_transitionLock) + { + sequenceIsOver = _reconnectExhaustedGeneration == generation; + } + + if (sequenceIsOver) + { + return; + } + + // Read once, before anything is announced, and used both for what is announced and for + // what is done about it. + bool willReconnect = !wasOpen || isNetworkDrop; + if (isNetworkDrop) { SetConnectionState( XrpConnectionState.RestoringConnection, message: "Network connection lost. Reconnecting...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); } else if (failedSession?.IsOpened == true) { @@ -2332,8 +3138,9 @@ private async Task OnConnectionFailed( SetConnectionState( XrpConnectionState.RestoringConnection, $"Connection lost: {error.Message}. Reconnecting...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); } else if (IsReconnectActive()) { @@ -2341,21 +3148,39 @@ private async Task OnConnectionFailed( SetConnectionState( XrpConnectionState.RestoringConnection, $"Connection attempt failed: {error.Message}", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); } else { - // True initial connection failure - no reconnect in progress + // A connection that never came up, and one the client is about to dial again: this + // path is only reached for a socket that never opened as a session, and a socket that + // never opened is always retried - willReconnect below reads the same fact. So no + // reason is named here, and there is no reason to name: a reason means the client + // stopped, which is what ConnectionStopReason.None exists to distinguish, and naming + // one while the retry is about to start would hand a consumer grounds to fail over to + // another server while this one is still being dialled. + // + // A ConnectionStopReason.InitialConnectionFailed existed here and was removed before + // release rather than kept as a defensive value. It is unreachable by construction, + // and that was measured rather than argued: the branch was instrumented and the whole + // unit suite run twice - once on the reason, once on willReconnect itself - for zero + // hits in 1328 tests. A value no consumer can ever observe is worse than no value, + // because it invites a branch that never runs. Adding an enum member later is not a + // breaking change; removing one is, which is why this is the right order to be wrong + // in. The ending a consumer does get for a connection that never came up and stopped + // being retried is ReconnectExhausted, from the loop that stopped retrying it. SetConnectionState( XrpConnectionState.Disconnected, $"Initial connection failed: {error.Message}", - ConnectionCloseSeverity.Error); + severity: ConnectionCloseSeverity.Error, + announcingGeneration: generation); } // 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 (willReconnect) { if (OnDisconnect is not null) { @@ -2504,7 +3329,7 @@ private async Task EnsureConnectionForRequest(RequestFailurePolicy? policyOverri // 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( + throw new RequestRefusedException( "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."); @@ -2527,7 +3352,22 @@ private void CheckIfNotConnected() { if (_permanentlyDisconnected) { - throw new NotConnectedException("Client has been disconnected. Call Connect() to reconnect."); + throw DisconnectedBecause("Client has been disconnected. Call Connect() to reconnect."); + } + + // The same record the wait reads, and read here for the same reason: a client that gave up + // and a client nobody has called Connect() on are indistinguishable by socket, loop and + // state, and the check below answers both with "call Connect() first". A consumer who has + // just been told on the status stream that this endpoint spent its reconnect budget, and + // who then issues a request, was told by the exception that they had never connected - + // which is the one distinction the exception family exists to draw. + lock (_transitionLock) + { + NotConnectedException? announced = StoppedBecauseLocked(); + if (announced != null) + { + throw announced; + } } // Connecting or RestoringConnection say an attempt is under way even with ws null. So @@ -2539,7 +3379,7 @@ private void CheckIfNotConnected() var noConnectionAttemptActive = ws == null && _reconnectCts == null && !isActiveState; if (noConnectionAttemptActive) { - throw new NotConnectedException("No connection attempt in progress. Call Connect() first."); + throw new NotConnectingException("No connection attempt in progress. Call Connect() first."); } } @@ -2679,6 +3519,10 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) try { connectionManager.ResolveAllAwaiting(); + + // Before the OnConnected handler, which is consumer code and may take its time: a + // caller waiting for the connection is waiting for the socket, not for the handler. + WakeConnectionWaiters(); if (OnConnected is not null) { await OnConnected?.Invoke(); @@ -2695,7 +3539,10 @@ private async Task OnceOpen(WebSocketClient connectedSocket, long sessionId) } Interlocked.Exchange(ref _connectHandlerFailures, value: 0); - SetConnectionState(XrpConnectionState.Connected, message: $"Connected {url}"); + SetConnectionState( + XrpConnectionState.Connected, + message: $"Connected {url}", + announcingGeneration: openedSession.Generation); } catch (Exception error) { @@ -2771,11 +3618,39 @@ await errorHandler // The detailed reason has to be notified BEFORE Disconnect(): Disconnect() moves the state to // Disconnected itself, and SetConnectionState only notifies on a state change, so a call after it // would be swallowed and the consumer would see "Disconnected by user request." instead. + // The generation goes with it, like every other announcement that speaks for one + // transition. The ownership check above is read-only and has no await after it, but a + // takeover on another thread can still land between the two - and this one announces a + // terminal reason, which the deduplication no longer swallows. + ConnectHandlerFailure gaveUp = new ConnectHandlerFailure( + $"Gave up connecting to {url}: the OnConnected handler failed {failures} time(s) in a row. " + + $"Call Connect() to retry.", + failures, + error); + + // Recorded before the ending is announced, because the announcement is the first thing + // anyone can see it by. A status handler asking what happened - and the caller the + // rejection below resumes - reads the cause through this field, and until it was + // written here they were told the consumer had disconnected the client: the one thing + // that did not happen, and the confusion the whole exception family exists to end. The + // disconnect further down is handed the same value, so the two cannot drift; a + // takeover landing in between replaces or clears it, which is correct, because then + // this path no longer speaks for the connection. + lock (_transitionLock) + { + if (Owns(failedSession.Generation)) + { + _connectHandlerGaveUp = gaveUp; + } + } + SetConnectionState( XrpConnectionState.Disconnected, message: $"OnConnected handler failed {failures} time(s) in a row: {error.Message}. Giving up after {config.MaxReconnectAttempts} attempts. Call Connect() to retry.", - ConnectionCloseSeverity.Error); + severity: ConnectionCloseSeverity.Error, + stopReason: ConnectionStopReason.ConnectHandlerFailed, + announcingGeneration: failedSession.Generation); // The notification above ran consumer code. A handler that answered "gave up" with a // ChangeServer has already taken this socket out of ws and is opening another; the @@ -2795,19 +3670,23 @@ await errorHandler // SetNetworkId sends straight after, and the socket really does open for a moment // before a failing handler brings it down. A caller that got as far as the second // operation was told its own request had been cancelled, having cancelled nothing. - requestManager.RejectAll(new NotConnectedException( - $"Gave up connecting to {url}: the OnConnected handler failed {failures} time(s) in a row. " + - $"Call Connect() to retry.")); - - await Disconnect(); + requestManager.RejectAll(new ConnectHandlerFailedException(gaveUp.Message, gaveUp.Failures, gaveUp.Error)); + + // The cause travels with the disconnect this path performs. Everything that reports the + // resulting state - a caller parked in WaitForConnectionAsync, an operation this + // disconnect supersedes, the next request - then says the handler failed instead of + // saying the consumer disconnected the client, which is the one thing that did not + // happen here. + await DisconnectAsync(gaveUp); return; } SetConnectionState( XrpConnectionState.RestoringConnection, message: $"OnConnected handler failed: {error.Message}. Reconnecting...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo(failures)); + severity: ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo(failures), + announcingGeneration: failedSession.Generation); // Always tear down the socket the handler actually ran for. WebSocketClient.Connect invokes its // OnConnect callback without awaiting it, so the connect lock can be released while this method is @@ -2846,7 +3725,10 @@ await errorHandler return; } - requestManager.RejectAllWithCancellation(); + // The handler failed and the client will try again: for the request that died with the + // connection this is a rebuild, not "the handler is broken" - that answer belongs to the + // terminal branch above, which gives up. + requestManager.RejectAll(SweptBy(ConnectionTransitionKind.Reconnect, url)); await AwaitMessageProcessorExitAsync(detachedProcessor.task, detachedProcessor.cts); await WaitForPingToFinishAsync(); @@ -3038,7 +3920,12 @@ await NotifySessionEndedAsync( if (intentionalDisconnect) { var noReconnectMessage = $"Connection closed permanently. {userMessage}"; - SetConnectionState(XrpConnectionState.Disconnected, noReconnectMessage, ConnectionCloseSeverity.Warning); + SetConnectionState( + XrpConnectionState.Disconnected, + noReconnectMessage, + severity: ConnectionCloseSeverity.Warning, + stopReason: ConnectionStopReason.ClosedPermanently, + announcingGeneration: closingGeneration); return; } @@ -3054,8 +3941,9 @@ await NotifySessionEndedAsync( SetConnectionState( XrpConnectionState.RestoringConnection, userMessage, - severity, - reconnect: firstAttempt); + severity: severity, + reconnect: firstAttempt, + announcingGeneration: closingGeneration); } } else @@ -3069,7 +3957,12 @@ await NotifySessionEndedAsync( } var noReconnectMessage = $"Connection closed permanently. {userMessage}"; - SetConnectionState(XrpConnectionState.Disconnected, noReconnectMessage, ConnectionCloseSeverity.Warning); + SetConnectionState( + XrpConnectionState.Disconnected, + noReconnectMessage, + severity: ConnectionCloseSeverity.Warning, + stopReason: ConnectionStopReason.ClosedPermanently, + announcingGeneration: closingGeneration); } } @@ -3099,6 +3992,14 @@ private bool StartReconnectLoop(long generation, int initialAttempts = 0) return false; } + // This generation already spent its budget and said so. Anything still arriving for it + // - the close of its last failed attempt above all - is the tail of a sequence that is + // over, not the start of a new one. + if (_reconnectExhaustedGeneration == generation) + { + return false; + } + // Set reconnect mode to LoopReconnect (upgrades from FastReconnect or sets from None) _reconnectMode = ReconnectMode.LoopReconnect; @@ -3199,10 +4100,30 @@ private async Task ReconnectLoopAsync(long generation, CancellationTokenSource o { if (config.StopAfterMaxAttempts) { + // Recorded before the notification, for the same reason the loop is started + // before one: the notification runs consumer code, and the close of the + // attempt that just failed can land while it does. Either would otherwise find + // a connection that looks like it has no sequence running. + lock (_transitionLock) + { + if (Owns(generation)) + { + _reconnectExhaustedGeneration = generation; + } + } + + // The generation goes with it. Without it this one announcement was the only + // one in the method that spoke for the sequence without saying whose it was: + // a takeover landing between the check above and this line left the loop + // correctly declining to record the exhaustion and then stamping a terminal + // "gave up" over the Connecting the new transition had just reported. SetConnectionState( XrpConnectionState.Disconnected, message: $"Reconnection stopped after {config.MaxReconnectAttempts} attempts.", - ConnectionCloseSeverity.Error); + severity: ConnectionCloseSeverity.Error, + stopReason: ConnectionStopReason.ReconnectExhausted, + announcingGeneration: generation); + break; } @@ -3214,8 +4135,9 @@ private async Task ReconnectLoopAsync(long generation, CancellationTokenSource o SetConnectionState( XrpConnectionState.RestoringConnection, reconnectMessage, - type, - reconnect: BuildReconnectInfo(delay: delay)); + severity: type, + reconnect: BuildReconnectInfo(delay: delay), + announcingGeneration: generation); if (!skipDelay) { @@ -3336,11 +4258,16 @@ private async Task ReconnectLoopAsync(long generation, CancellationTokenSource o var errorMessage = isNetworkError ? $"Reconnection attempt #{_reconnectAttempts}: network unavailable" : $"Reconnection attempt #{_reconnectAttempts} failed: {ex.Message}"; + // The ownership check above decides whether to carry on; this decides whether the + // status is still this loop's to report, and the two are not the same instant. A + // takeover landing between them - a user Disconnect above all - had its own state + // overwritten with a RestoringConnection from a loop that was already superseded. SetConnectionState( XrpConnectionState.RestoringConnection, errorMessage, - severity, - reconnect: BuildReconnectInfo()); + severity: severity, + reconnect: BuildReconnectInfo(), + announcingGeneration: generation); } } @@ -3377,6 +4304,13 @@ private async Task ReconnectLoopAsync(long generation, CancellationTokenSource o } finished?.Dispose(); + + // The state a waiter reads to recognise a spent budget is this bookkeeping, not the + // notification that preceded it: the check is "the budget is gone AND no source is + // installed", and the source is only released here. A waiter woken by the notification + // alone re-reads a connection that still has one, finds nothing terminal, and parks again + // on a signal nothing else was going to complete. + WakeConnectionWaiters(); } private volatile int _pingRunning = 0; @@ -3751,6 +4685,11 @@ private static (ConnectionCloseSeverity severity, string message) DescribeClose( 1009 => (ConnectionCloseSeverity.Warning, "Message too large (1009)." + suffix), 1010 => (ConnectionCloseSeverity.Error, "Mandatory WebSocket extension is missing (1010)." + suffix), 1011 => (ConnectionCloseSeverity.Error, "Internal server error (1011)." + suffix), + // A close with no code at all reaches the default arm too, and interpolating a null + // int? there produced "Connection closed with code ." - a sentence with a hole in it, + // reported for the commonest close of all: a peer that went away without a frame. + null => (ConnectionCloseSeverity.Warning, "Connection closed without a code." + suffix), + _ => (ConnectionCloseSeverity.Warning, $"Connection closed with code {code}." + suffix), }; } diff --git a/Xrpl/Xrpl.csproj b/Xrpl/Xrpl.csproj index 0c70dc24..d3e48208 100644 --- a/Xrpl/Xrpl.csproj +++ b/Xrpl/Xrpl.csproj @@ -14,7 +14,7 @@ Apache-2.0 https://github.com/StaticBit-io/XrplCSharp XrplCSharp - 11.4.0.0 + 11.5.0.0 diff --git a/XrplCSharp.sln b/XrplCSharp.sln index 5974f42a..9f7f5602 100644 --- a/XrplCSharp.sln +++ b/XrplCSharp.sln @@ -52,6 +52,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "X402.PayingClient", "Exampl EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Xrpl.GenerateEnums.Test", "Tests\Xrpl.GenerateEnums.Test\Xrpl.GenerateEnums.Test.csproj", "{D282107C-A4BC-4E9C-B791-08D256A541B4}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ConnectionLifecycleSample", "Tests\TestsClients\ConnectionLifecycleSample\ConnectionLifecycleSample.csproj", "{4CB1F91A-84F4-4545-A697-5515B3DBB16E}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -266,6 +268,18 @@ Global {D282107C-A4BC-4E9C-B791-08D256A541B4}.Release|x64.Build.0 = Release|Any CPU {D282107C-A4BC-4E9C-B791-08D256A541B4}.Release|x86.ActiveCfg = Release|Any CPU {D282107C-A4BC-4E9C-B791-08D256A541B4}.Release|x86.Build.0 = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|Any CPU.Build.0 = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|x64.ActiveCfg = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|x64.Build.0 = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|x86.ActiveCfg = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Debug|x86.Build.0 = Debug|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|Any CPU.ActiveCfg = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|Any CPU.Build.0 = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|x64.ActiveCfg = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|x64.Build.0 = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|x86.ActiveCfg = Release|Any CPU + {4CB1F91A-84F4-4545-A697-5515B3DBB16E}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -276,6 +290,7 @@ Global {BFA55E11-9F4D-4761-8C61-AE15DCFFDBE5} = {0151507B-5C0A-4F16-A211-631A1D446E94} {3E1FF668-30C8-46B4-A32A-1C7679F4C0AA} = {A1A18B80-6C6A-40CB-9B55-48B1C902CECC} {3BAE22CE-82E4-497E-BE42-893B226291BE} = {99A4D9AF-9B2F-4812-A882-F0A461D8721F} + {4CB1F91A-84F4-4545-A697-5515B3DBB16E} = {99A4D9AF-9B2F-4812-A882-F0A461D8721F} {713B2FD4-695A-409E-B90A-8D5EA78F95E9} = {A1A18B80-6C6A-40CB-9B55-48B1C902CECC} {59CA8C39-D541-4415-858B-3A1DEDA7FD43} = {A1A18B80-6C6A-40CB-9B55-48B1C902CECC} {6F8F3534-A71D-4F70-B353-576A799D839D} = {A1A18B80-6C6A-40CB-9B55-48B1C902CECC} diff --git a/specs/2026-09-09-connection-outcome-api.md b/specs/2026-09-09-connection-outcome-api.md new file mode 100644 index 00000000..df3bca02 --- /dev/null +++ b/specs/2026-09-09-connection-outcome-api.md @@ -0,0 +1,1182 @@ +# Результат перехода соединения читается типом, а не текстом + +Дата: 2026-09-09. Статус: **реализовано**, ревизия 4. Один PR, один релиз. +Базис: 11.4.0.0 (после #181). + +**Открытых вопросов нет.** Всё, что относится к теме, решено здесь и едет одним изменением: +типы исключений, свипы запросов в полёте (2.6), незащищённый `SetNetworkId` на пути +`ChangeServer` (2.7), событийное ожидание готовности (3.2) и причина останова в потоке статуса +(4). Возвращаться к этому коду второй раз не потребуется. + +Источник: запрос от двух команд, использующих этот SDK. Запрос принят по существу; +расхождения с ним перечислены в разделе 6. + +> **Ревизия 2.** Первая редакция прошла два независимых ревью (Fable 5.1 с полным доступом к +> файлам, Codex по выдержкам без доступа к репозиторию). Оба нашли одно и то же ядро проблем, +> и все находки проверены по коду перед внесением. Что изменилось: +> - раздел 2.3 первой редакции **сужал** внутреннюю проверку на `connection.cs:1339` до двух +> новых типов. Это вернуло бы дефект «вторая серия переподключения», исправленный в 11.4.0.0: +> `WaitForConnectionAsync` бросает и базовый `NotConnectedException` — через +> `CheckIfNotConnected()` на входе. Сужение убрано, раздел переписан на обратное утверждение; +> - «инвариант 1:1» между перечислением исходов и типами исключений был ложен в обе стороны. +> Заменён областью действия с полным перечислением (раздел 3.1); +> - `ClientDisconnectedException` смешивал пользовательское отключение с внутренним, которым +> заканчивается отказ на `OnConnected`-обработчике. В репозитории уже есть два теста на один +> сценарий, которые под первой редакцией дали бы разные типы в зависимости от тайминга. +> Введена причина отключения (раздел 2.3); +> - `ReconnectInfo.Exhausted` заменён на `ConnectionStatusInfo.StopReason`: признак ставится на +> объект, описывающий уведомление целиком, `Reconnect != null` сохраняет единственный смысл, и +> этим же закрывается бывший открытый вопрос 2 (раздел 4); +> - `HasConnectionAsync` на интерфейс не поднимается, перегрузка с `CancellationToken` не +> добавляется: она делает вызов без аргументов неоднозначным (CS0121) у потребителей; +> - добавлено предусловие реализации, которого не было: у `NotConnectedException` нет +> конструктора с `innerException`; +> - пункт 6.3 первой редакции утверждал, что `ChangeServer` не читает network id. Это неверно, +> предложение потребителей было право — см. 1.5. +> +> **Ревизия 3.** Три открытых вопроса второй редакции закрыты решениями, а не переносом: +> - свипы запросов в полёте больше не остаются голым `OperationCanceledException` (2.6); +> - предложенный потребителями готовый примитив ожидания **не берём** (3.2); +> - `XrplClient.ChangeServer` получает ту же защиту повторами, что и `Connect` (2.7). +> +> **Ревизия 4 — по итогам реализации.** Три места, где код разошёлся со спекой, и все три +> нашлись тестами, а не рассуждением: +> - **`Task.WhenAll` подтип не теряет** — оба ревьюера утверждали обратное, и тест, написанный +> по их формулировке, упал. Подтип теряется только рядом со сбойной задачей, и тогда отмена +> отбрасывается совсем (2.5); +> - **переиспользовать `ConnectionManager` как сигнал готовности нельзя**: он будит ожидающих на +> отставке соединения, а отставка обязана переносить запрос на новое соединение. Построен +> отдельный сигнал, и правило «пробуждение всегда перевзводит сигнал» — исправление дефекта, +> который поймал тест (3.2); +> - **фильтр повторов из 2.7 был слишком широким** дважды подряд: сначала исключал всё +> перекрытие, потом всё, кроме реконнекта. Верная граница — между жизненным циклом самого +> клиента и операцией-соседом (2.7). + +## 0. Порядок работ + +Одним PR, в этом порядке — каждый шаг опирается на предыдущий: + +1. **2.0** — конструктор `NotConnectedException(string, Exception?)`. Без него не собирается 2.1. +2. **2.1, 2.3** — новые типы и причина отключения. +3. **2.2** — переключение точек броска на новые типы. +4. **2.6** — свипы запросов в полёте. +5. **2.7** — `SetNetworkId` на пути `ChangeServer`. +6. **4** — `ConnectionStopReason` в потоке статуса. +7. **3.1** — `ConnectionWaitOutcome` и члены на `Connection`, `XrplClient`, `IXrplClient`. +8. **3.2** — событийное ожидание. Идёт последним: оно опирается на типы из 2.1, на причину из + 2.3 и на свипы из 2.6, и именно оно даёт единственное изменение внутренней механики. + +Тесты раздела 5 пишутся вместе с шагом, который они закрепляют, а не в конце. + +## 1. Проблема + +11.4.0 сделал поведение соединения правильным: у перехода один владелец, перекрытая операция +сообщает, что её перекрыли, вместо возврата успеха от сервера, который клиент покинул. Чего он +не сделал — не дал вызывающему **прочитать** этот ответ. + +Сегодня «что случилось с моим соединением» выразимо только парой «тип + текст сообщения». +Тип перегружен, текст — не контракт: changelog 11.3.2.0 сам предписывает «Classify by type +rather than by message», а типов, способных на это, библиотека не даёт. Внутренне +противоречивый контракт. + +### 1.1. Шесть значений `NotConnectedException` + +| Место | Значение | Что должен сделать потребитель | +|---|---|---| +| `connection.cs:885`, `:1392`, `:2530` | клиент отключён — но **чем**, по типу неизвестно (см. 1.4) | зависит от причины | +| `connection.cs:1399` | цикл переподключения исчерпал бюджет попыток | этот узел не отвечает; повод для failover | +| `connection.cs:2507` | запрос отклонён сразу по `RequestFailurePolicy.ImmediateFail` | повторить после подключения; узел ни при чём | +| `connection.cs:2798` | отказ на падающем `OnConnected`-обработчике, запросы в полёте | узел отвечает, отказал наш обработчик; failover не поможет | +| `connection.cs:2542` | попытки подключения нет вовсе | вызвать `Connect()` | +| `connection.cs:2516`, `:2522`, `:2315` | остаточные ветки, см. ниже | — | + +Про три остаточные ветки, чтобы они не выглядели пропущенными: + +- `:2516` — проверка `ShouldBeConnected()` **после** успешного возврата из ожидания. Это не + таймаут ожидания: ожидание уже вернулось. +- `:2522` — ветка `default` в `switch` по перечислению из двух значений. Недостижима. +- `:2315` — `connectionManager.RejectAllAwaiting(...)`, и путь мёртв: единственный потребитель + этого механизма, `ConnectionManager.AwaitConnection`, не вызывается нигде в `Xrpl/Client`. + +Все три остаются на базовом типе и в объём не входят. + +### 1.2. Перекрытие неотличимо от отмены вызывающим + +`SupersededLocked()` (`connection.cs:882`) уже делает `switch` по `_generationKind` и уже держит +`url` — то есть **конструирует правильный ответ и тут же теряет его** в голом +`OperationCanceledException`, где случай различим только по тексту. + +Четвёртая точка, `ChangeServer` после успешного ожидания (`connection.cs:1153`), идёт мимо +`SupersededLocked` и бросает такой же голый `OperationCanceledException` от себя. Её условие — +только `connectedTo != server`; вид перехода-победителя оно не читает, поэтому текст сообщения +(«superseded by a later ChangeServer») — вывод, а не факт. + +Для вызывающего всё это неотличимо от отмены собственным токеном. Потребители различали по +тексту или по внешнему флагу «я сам это отменил». + +### 1.3. Ожидание готовности: опрос и недоступная форма ответа + +`WaitForConnectionAsync` (`connection.cs:1361`) — цикл `Task.Delay(100 ms)`. Он медленнее +события, которого ждёт, а на Blazor WebAssembly дороже, чем выглядит: поток один, а таймеры в +скрытой вкладке троттлятся, и ожидание, которое событие удовлетворило бы мгновенно, +растягивается на секунды. + +Форма «не вернулось за отведённое время» — это ответ, а не сбой, и она в библиотеке **уже +есть**: `HasConnectionAsync` (`connection.cs:1426`). Но она ловит `System.TimeoutException` и +`OperationCanceledException`, а `NotConnectedException` пропускает наружу, и не принимает +`CancellationToken`. Потребитель, держащий `IXrplClient`, добирается до обеих только через +`client.connection` (`IXrplClient.cs:85`) — то есть через утечку всего внутреннего объекта. + +### 1.4. Отказ на обработчике неотличим от пользовательского отключения + +Путь «сдались на `OnConnected`-обработчике» отклоняет запросы в полёте своим сообщением +(`connection.cs:2798`), а затем **сам вызывает** `await Disconnect()` (`connection.cs:2802`). +`Disconnect()` ставит `_permanentlyDisconnected` (`connection.cs:773`), и после этого каждая +точка, которая читает этот флаг, отвечает так, будто клиента выключил потребитель: +`:1392` для ожидающего, `:885` для перекрытой операции, `:2530` для следующего запроса. + +Это не гипотеза: в репозитории уже есть два теста на один и тот же сценарий. +`TestUOnConnectedHandlerFailure.cs:312` — обработчик падает сразу, и `Connect()` завершается +через `:1392`. `TestUOnConnectedHandlerFailure.cs:192` — обработчик падает через 400 мс, +`Connect()` доходит до `SetNetworkId` и получает исключение из `:2798`. **Один сценарий, два +разных ответа в зависимости от тайминга** — ровно то, от чего это изменение должно избавить. + +### 1.5. Ловушка с `TimeoutException` живёт на пути `ChangeServer` + +`XrplClient.ChangeServer` (`IXrplClient.cs:907`) после `connection.ChangeServer` вызывает +`await SetNetworkId()` — то есть переключение сервера действительно спрашивает у нового узла +его network id, и этот запрос может завершиться `Xrpl.Client.Exceptions.TimeoutException`, +который не является `System.TimeoutException`. Предложение потребителей описало это верно. + +Отдельно стоит отметить, хотя в объём это не входит: `Connect` защищает тот же вызов повторами +через `SetNetworkIdWhileConnectingAsync` (`IXrplClient.cs:965`), а `ChangeServer` вызывает +`SetNetworkId()` напрямую, без повторов. Исправляется здесь же — см. 2.7. + +### 1.6. «Ещё пытаюсь» против «сдался» выводится из отсутствия + +Состояние `Disconnected` рассылается из десяти мест: `connection.cs:1792`, `:1803`, `:1856`, +`:1866`, `:2291`, `:2350`, `:2774`, `:3041`, `:3072`, `:3202`. Ни одно не передаёт `reconnect:`. +Уведомление «цикл сдался» неотличимо по форме от «пользователь отключился», «первое подключение +не удалось» и «закрыто окончательно» — отличается только текст. + +## 2. Решение, часть 1: типы вместо таблицы соответствий + +Не «перенести метод `Classify` в SDK». Таблица соответствий — обходной приём для типов, не +различающих то, что вызывающий обязан различать. Библиотека знает, какой это случай, в момент +броска. + +### 2.0. Предусловие реализации + +`NotConnectedException` имеет **единственный** конструктор `(string message = null)` +(`Exceptions/XrplException.cs:89`). Наследник, который должен нести исходную ошибку, передать её +некуда: у `InnerException` нет сеттера. Поэтому первым шагом: + +```csharp +public NotConnectedException(string message, Exception? innerException) + : base(message ?? DefaultMessage, innerException) { } +``` + +Базовый `XrplException` такой конструктор уже имеет (`Exceptions/XrplException.cs:31`), так что +правка на одну строку — но без неё раздел 2.1 нереализуем. + +### 2.1. Новые типы + +```csharp +// Все — наследники NotConnectedException: существующие catch продолжают ловить. +public class ClientDisconnectedException : NotConnectedException // потребитель вызвал Disconnect() +public class ReconnectExhaustedException : NotConnectedException // цикл исчерпал бюджет попыток +public class RequestRefusedException : NotConnectedException // ImmediateFail отклонил запрос +public class ConnectHandlerFailedException : NotConnectedException // отказ на OnConnected +public class NotConnectingException : NotConnectedException // попытки подключения нет + +// Наследник OperationCanceledException — по той же причине. +public class ConnectionSupersededException : OperationCanceledException +``` + +Носители данных, а не только имена: + +- `ReconnectExhaustedException`: `int Attempts`, `int MaxAttempts`. + **`Attempts` — число сделанных попыток, то есть равно `MaxAttempts`.** Определить это + обязательно: `_reconnectAttempts` инкрементируется в начале витка (`connection.cs:3187`), + цикл останавливается при `> MaxReconnectAttempts` (`:3198`) и счётчик на выходе не сбрасывает, + так что сырое значение в точке броска равно `MaxAttempts + 1`. Публиковать «6 из 5» нельзя. +- `ConnectHandlerFailedException`: `int Failures` и исходная ошибка во `InnerException` + (см. 2.0). Обе величины есть на `connection.cs:2798`. +- `ConnectionSupersededException`: `string? SupersededBy` — где операция-победитель оставила + клиента, — и `ConnectionTransitionKind Kind`: + +```csharp +public enum ConnectionTransitionKind +{ + Connect, + ChangeServer, + Disconnect, + Reconnect, // FastReconnect приватного TransitionKind +} +``` + +`None` приватного `TransitionKind` в перечисление не входит: значение означает «перехода нет», +и в точке броска оно недостижимо. + +`Disconnect` **достижим только на свипах запросов** (2.6). На пути перекрытого перехода +`TransitionKind.Disconnect` даёт `ClientDisconnectedException`, а не +`ConnectionSupersededException`, — так это работает сегодня, и менять наблюдаемый тип там +нельзя. Асимметрия не случайная: каждая половина сохраняет тот тип, который вызывающий получает +сейчас. Это записано в XML-документации `Kind`, чтобы потребитель не искал недостижимую ветвь и +не удивлялся достижимой. + +### 2.2. Точки броска + +| Файл:строка | Было | Станет | +|---|---|---| +| `connection.cs:885` | `NotConnectedException` | `ClientDisconnectedException` или `ConnectHandlerFailedException` — по причине (2.3) | +| `connection.cs:886–888` | `OperationCanceledException` | `ConnectionSupersededException` (`Kind` из `_generationKind`, `SupersededBy = url`) | +| `connection.cs:1153` | `OperationCanceledException` | `ConnectionSupersededException`, `Kind` читается из `_generationKind` **под `_transitionLock`**, `SupersededBy = connectedTo` | +| `connection.cs:1392` | `NotConnectedException` | `ClientDisconnectedException` или `ConnectHandlerFailedException` — по причине (2.3) | +| `connection.cs:1399` | `NotConnectedException` | `ReconnectExhaustedException` | +| `connection.cs:2507` | `NotConnectedException` | `RequestRefusedException` | +| `connection.cs:2530` | `NotConnectedException` | `ClientDisconnectedException` или `ConnectHandlerFailedException` — по причине (2.3) | +| `connection.cs:2542` | `NotConnectedException` | `NotConnectingException` | +| `connection.cs:2798` | `NotConnectedException` | `ConnectHandlerFailedException` | + +`:2516`, `:2522` и `:2315` остаются на базовом `NotConnectedException` (см. 1.1). + +На `:1153` вид **читается**, а не подставляется константой: условие там доказывает только +несовпадение адресов. Сегодня по построению победителем оказывается `ChangeServer` (`Connect()` +и быстрый реконнект идут на текущий `url`, то есть дали бы `connectedTo == server`), но это +вывод из чужого кода, а не факт, и превращать его в структурированные публичные данные нельзя. + +Тексты сообщений сохраняются дословно. Смысл переносится в тип, а не переписывается в строке. + +### 2.3. Причина отключения + +Чтобы 1.4 перестало быть правдой, отключение должно нести причину: + +```csharp +private enum DisconnectCause +{ + User, // Disconnect() / DisconnectAndWaitAsync() от потребителя + ConnectHandlerGaveUp, // путь connection.cs:2774-2802 +} +``` + +Причина записывается **в той же критической секции, где ставится +`_permanentlyDisconnected`** (`connection.cs:773`), и живёт ровно столько же — отдельной логики +сброса нет, а значит нет и способа её рассинхронизировать. Практически это означает внутренний +параметр у `Disconnect()`, которым путь отказа на обработчике (`connection.cs:2802`) передаёт +`ConnectHandlerGaveUp`, а все публичные вызовы — `User` по умолчанию. + +Читают её три точки — `:885`, `:1392`, `:2530` — и бросают +`ConnectHandlerFailedException` (с `Failures` и `InnerException`) вместо +`ClientDisconnectedException`, когда причина `ConnectHandlerGaveUp`. + +### 2.4. Почему внутренняя проверка остаётся широкой + +`connection.cs:1339` — `if (ex is NotConnectedException)` в обработчике ошибок быстрого +реконнекта — **не сужается**. Первая редакция этой спеки предлагала заменить проверку на пару +новых типов, обосновывая это тем, что «`WaitForConnectionAsync` в этой точке может бросить +только их». Это неверно: ожидание на входе вызывает `CheckIfNotConnected()` +(`connection.cs:1368`), а тот бросает базовый `NotConnectedException` на `:2542` — под этой +спекой `NotConnectingException`, всё ещё наследник базы, но не один из двух названных. + +Путь достижим. При `MaxReconnectAttempts = 1`: `ConnectCoreAsync` падает на сокете → +`OnConnectionFailed` запускает цикл на том же источнике отмены → цикл инкрементирует счётчик, +получает `2 > 1`, объявляет `Disconnected` и на выходе обнуляет `_reconnectCts` и +`_reconnectLoopGeneration` (`connection.cs:3375`) → если это успевает до входа в +`WaitForConnectionAsync`, `CheckIfNotConnected` видит `ws == null && _reconnectCts == null` при +состоянии `Disconnected` и бросает `:2542`. Сегодня широкая проверка это ловит и останавливается. +Сузив её, мы отправили бы исключение в `StartReconnectLoop`, чей guard по поколению уже сброшен — +то есть запустили бы вторую серию за терминальным `Disconnected`. Это ровно тот дефект, который +закрыт в 11.4.0.0 («a fast reconnect no longer runs a second full series after the loop gave +up»). + +Проверка семантически верна как есть: **любой** `NotConnectedException` из ожидания означает +«клиент не подключается, вторую серию не запускать». Комментарий на `:1335–1338` уточняется, +код не меняется. + +### 2.5. Что даёт наследование от `OperationCanceledException` и чего не даёт + +`AsyncTaskMethodBuilder.SetException` переводит задачу в `Canceled`, когда исключение — +`OperationCanceledException`, независимо от токена. Так ведёт себя `ChangeServer` уже сегодня; +наследник ведёт себя так же, `catch (OperationCanceledException)` продолжает ловить. Записано, +чтобы это не «чинили». + +Чего наследование не даёт, и что должно попасть в XML-документацию нового типа: + +- подтип сохраняется при **прямом** `await` задачи. **Уточнено замером** (`TestUConnectionOutcomeTypes`), + и замер опроверг ожидание, с которым тест писался: `Task.WhenAll` подтип тоже сохраняет, пока + среди задач нет сбойных — комбинированная задача хранит первое исключение отмены и бросает + именно его. Теряется он в другом случае: если хоть одна задача **сбойная**, `WhenAll` + записывает только сбои, а отмену отбрасывает совсем — её нет ни в `await`, ни в + `Task.Exception.InnerExceptions`. Писать пессимистичное правило («через `WhenAll` подтип + теряется всегда») значило бы отправить потребителя искать обходной путь, который ему не нужен; +- конструктор передаёт `CancellationToken.None`: иначе фильтры вида + `when (ex.CancellationToken == myToken)` начнут срабатывать на перекрытии, которого + вызывающий не отменял. +- `catch (TaskCanceledException)` новый тип не поймает — он ему не наследник. Это правильно и + неочевидно. +- внутри SDK новый тип попадает в существующие `catch (OperationCanceledException)`: + `connection.cs:1492` (`Connect` глотает — желаемо), `:3319` (цикл — желаемо), + `IXrplClient.cs:977` (`SetNetworkIdWhileConnectingAsync` повторяет — желаемо). Проверено, в + тестах закрепить. +- **`TaskCompletionSource.SetException(OperationCanceledException)` даёт `Faulted`, а не + `Canceled`.** Правило асинхронного билдера на TCS не распространяется — это важно для этапа + 2b (3.2), где ожидание переводится на TCS. + +### 2.6. Свипы запросов в полёте + +Потребители из предложения держат `IXrplClient` и чаще всего сталкиваются не с перекрытым +переходом, а с **собственным запросом, который умер, пока соединение переезжало**. Такой запрос +сегодня отклоняется через `RequestManager.RejectAllWithCancellation()` — голым +`OperationCanceledException("Connection was intentionally closed.")` +(`RequestManager.cs:246`) — и это ровно тот случай, о котором changelog 11.3.2.0 пишет «or +`OperationCanceledException`, for a request that was in flight when the switch began». Оставить +его голым значит закрыть половину задачи. + +**Правило.** Свип, который делает **переход**, забирающий соединение куда-то, сообщает куда. +Свип, вызванный **отказом самого соединения**, остаётся отменой — намеренно, см. ниже. + +Механизм уже есть: `RequestManager.RejectAll(Exception)` (`RequestManager.cs:232`) и +`ConnectionManager.RejectAllAwaiting(Exception)` (`ConnectionManager.cs:23`) принимают готовое +исключение. Добавляется одна приватная фабрика рядом с `SupersededLocked` — тот же `switch`, +но для свипа, — и каждая точка передаёт свой вид перехода. + +| Точки | Кто делает свип | Чем отклоняется | +|---|---|---| +| `:1101`, `:1102` | `ChangeServer` | `ConnectionSupersededException(ChangeServer, SupersededBy = server)` | +| `:1245`, `:1246` | быстрый реконнект | `ConnectionSupersededException(Reconnect, SupersededBy = url)` | +| `:1469` | `Connect` | `ConnectionSupersededException(Connect, SupersededBy = url)` | +| `:1780`, `:1781` | `Disconnect` | `ConnectionSupersededException(Disconnect, SupersededBy = null)` | +| `:1831`, `:1832` | `DisconnectAndWaitAsync` | `ConnectionSupersededException(Disconnect, SupersededBy = null)` | +| `:2849` | отказ обработчика, ветка повтора | `ConnectionSupersededException(Reconnect, SupersededBy = url)` | + +Все шесть остаются наследниками `OperationCanceledException`, поэтому **ни один существующий +`catch (OperationCanceledException)` не перестаёт срабатывать и ни одна задача не меняет +статус**. Меняется только то, что теперь можно прочитать. + +`:1780`/`:1831` намеренно дают `ConnectionSupersededException(Disconnect)`, а не +`ClientDisconnectedException`: последний — наследник `NotConnectedException`, и запрос, который +сегодня умирает отменой, начал бы умирать сбоем. Ради читаемости типа ломать это нельзя, а +`Kind = Disconnect` отвечает на вопрос полностью. + +`:2849` — ветка, где обработчик `OnConnected` упал, но клиент будет пробовать снова; запрос +умер из-за пересоздания соединения, поэтому `Reconnect`, а не «обработчик сломан». Терминальная +ветка того же пути (`:2798`) остаётся `ConnectHandlerFailedException`, как в 2.2. + +**Что остаётся отменой и почему.** Свипы на `:2290`/`:2291`, `:2310`/`:2311` и `:2995` вызваны +не переходом, а закрытием сокета — сетевым обрывом или закрытием, которое уже отчиталось само. +Комментарий на `:2992` фиксирует это как решение: отмена выбрана, чтобы приложения-потребители +не писали такие случаи в Critical. Решение остаётся в силе, а `StopReason` (раздел 4) даёт +причину на потоке статуса, где ей и место. `:2315` не трогается — путь мёртв (1.1). + +### 2.7. `SetNetworkId` на пути `ChangeServer` + +`XrplClient.Connect` защищает чтение network id повторами через +`SetNetworkIdWhileConnectingAsync` (`IXrplClient.cs:965`), потому что соединение сразу после +подключения может быть ещё неустоявшимся. `XrplClient.ChangeServer` (`IXrplClient.cs:916`) +вызывает `SetNetworkId()` напрямую. Переключение сервера подвержено ровно тому же: сокет +открылся, `WaitForConnectionAsync` вернулся, а `OnConnected`-обработчик или конкурентный +переход уже уносит соединение — и `server_info` умирает вместе с ним. + +`:916` заменяется на `await SetNetworkIdWhileConnectingAsync(cancellationToken)`. + +Одновременно уточняется фильтр повторов на `IXrplClient.cs:976`. Сейчас он ловит +`OperationCanceledException or DisconnectedException`; после 2.1 в `OperationCanceledException` +попадает и `ConnectionSupersededException`, а после 2.6 — ещё и свипы запросов. + +**Исключать `ConnectionSupersededException` целиком нельзя** — это выяснилось прогоном +существующего теста `TestRecoveredHandlerFailureWithRequestInFlightStillConnects`, который +упал на первой редакции правки. Пересборка соединения (свип из 2.6 с +`Kind == Reconnect` — реконнект health-check'а или повтор после один раз упавшего +`OnConnected`) — это ровно тот случай, ради которого цикл повторов и существует: клиент +возвращается на тот же сервер. Перекрытие **потребительской** операцией — не тот: соединение +теперь принадлежит ей. + +Фильтр решает по виду перехода: + +```csharp +private static bool IsWorthAnotherNetworkIdAttempt(Exception error) => + error switch + { + ConnectionSupersededException superseded => + superseded.Kind == ConnectionTransitionKind.Reconnect, + OperationCanceledException => true, + DisconnectedException => true, + _ => false, + }; +``` + +**Граница уточнена повторно, тоже прогоном тестов.** Исключать по признаку «не реконнект» тоже +неверно: путь отказа на обработчике заканчивается собственным `Disconnect()`, и запрос на +network id гибнет в его свипе — то есть под `Kind == Disconnect`. Пробросив это, `Connect()` +сообщал бы случайный свип вместо причины, по которой клиент сдался, причём **в зависимости от +тайминга**: при быстром событийном ожидании приходило одно, при медленном опросе — другое. + +Верная граница — между собственным жизненным циклом клиента и операцией-соседом. `Reconnect` и +`Disconnect` переспрашиваем: ожидание внутри следующей попытки сообщит настоящую причину +(`ConnectHandlerFailedException`, `ClientDisconnectedException`). `Connect` и `ChangeServer` +пробрасываем: соединение принадлежит той операции, и «вашу операцию перекрыли» — это весь +ответ. + +## 3. Решение, часть 2: ожидание готовности + +### 3.1. Форма ответа и интерфейс + +Вместо предложенного `Task` — перечисление. Это ровно та же мысль, что и в части 1, но на +неисключительном пути: «не вернулось за отведённое время», «сдался» и «попытки нет» — разные +ответы, и сводить их в один `false` значит воспроизвести исходную проблему в новом методе. + +```csharp +public enum ConnectionWaitOutcome +{ + Connected, // соединение установлено + TimedOut, // не вернулось за отведённое время + ReconnectExhausted, // цикл исчерпал бюджет попыток + Disconnected, // потребитель вызвал Disconnect() + ConnectHandlerFailed, // отказ на OnConnected-обработчике + NotConnecting, // попытки подключения нет — нужен Connect() +} + +// на Connection, на XrplClient и на IXrplClient (default-реализация, как у DroppedStreamMessages) +Task WaitForConnectionOutcomeAsync( + TimeSpan? timeout = null, + CancellationToken cancellationToken = default); +``` + +**Область действия вместо инварианта.** Первая редакция объявляла соответствие 1:1 между +перечислением и типами из 2.1. Это было ложно в обе стороны: `TimedOut` соответствует +`System.TimeoutException`, который раздел 7 намеренно оставляет типом BCL, а +`RequestRefusedException` и `ConnectionSupersededException` относятся к допуску запроса и +владению переходом, а не к ожиданию готовности, и парного значения не имеют. + +Соответствие определено **только для того, что бросает `WaitForConnectionAsync`**, и полностью: + +| Исключение | Точка | Значение | +|---|---|---| +| `ClientDisconnectedException` | `:1392`, и `:2530` через `:1368` | `Disconnected` | +| `ConnectHandlerFailedException` | те же точки при причине `ConnectHandlerGaveUp` | `ConnectHandlerFailed` | +| `ReconnectExhaustedException` | `:1399` | `ReconnectExhausted` | +| `NotConnectingException` | `:2542` через `:1368` | `NotConnecting` | +| `System.TimeoutException` | `:1406` | `TimedOut` | +| `OperationCanceledException` (токен вызывающего) | `:1412`, `:1421` | **пробрасывается** | +| `ArgumentOutOfRangeException` (некорректный таймаут) | `:1374` | **пробрасывается** | + +Отмена собственным токеном и некорректный аргумент остаются исключениями: первое — конвенция +.NET, второе — ошибка вызывающего, а не результат работы соединения. + +**`HasConnectionAsync` не трогаем и на интерфейс не поднимаем.** Перегрузка +`HasConnectionAsync(TimeSpan? = null, CancellationToken = default)` рядом с существующей +`HasConnectionAsync(TimeSpan? = null)` делает вызов без аргументов неоднозначным: обе +перегрузки применимы, обеим нужна подстановка умолчаний, правило «лучшего члена» не выбирает — +CS0121. В репозитории вызовов без аргументов нет, поэтому сборка бы прошла, а сломались бы +потребители — при заявленной аддитивности. А переопределение её через +`outcome == Connected` изменило бы наблюдаемое поведение: сегодня она пропускает +`NotConnectedException` наружу, и существующие `catch` у потребителей перестали бы срабатывать. +Метод остаётся как есть, на `Connection`; на интерфейс идёт один член на одно понятие — +`WaitForConnectionOutcomeAsync`. + +**Default-реализация и класс.** Default-член интерфейса вызывается только через ссылку типа +`IXrplClient`; через ссылку типа `XrplClient` он не виден. Поэтому метод добавляется и в класс — +так же, как сделано с `DroppedStreamMessages` (`IXrplClient.cs:744`). Обе формы вызова +закрепляются тестами. + +### 3.2. Событие вместо опроса + +**Реализовано; описание ниже — того, что построено, а не того, что планировалось.** Первая +редакция этого раздела предлагала переиспользовать `ConnectionManager` — примитив ожидания, +который в SDK уже есть, уже разослан по жизненному циклу и у которого не хватает только +слушателя (`AwaitConnection()` не вызывается нигде). От этого пришлось отказаться при +реализации, и причина важнее самой правки. + +**Почему `ConnectionManager` не подходит.** Его `RejectAllAwaiting*` вызывается в том числе на +шести точках **отставки** соединения. Но отставка — не повод прекращать ждать: клиент +пересобирает соединение, и запрос под `RequestFailurePolicy.WaitForConnection` обязан +перенестись на новое соединение. Это записанный контракт (changelog 11.3.2.0: «`WaitForConnection` +carries it over to the new connection»), и разбудив ожидающего на отставке, мы превратили бы +перенос в отказ. Сегодня это незаметно ровно потому, что `AwaitConnection` никто не слушает. + +**Что построено вместо.** Отдельный сигнал готовности на `TaskCompletionSource`: + +- завершается значением, а не исключением. Ждать может никто, а завершённая исключением задача + без наблюдателя — это unobserved exception; причину по-прежнему строит точка броска; +- `RunContinuationsAsynchronously` обязателен: сигнал завершается изнутри критических секций, + двигающих соединение, и без него каждый ожидающий возобновлялся бы прямо там — дефект того же + класса, что #177; +- **пробуждение всегда перевзводит сигнал** — в той же критической секции, что и завершение. + Это не украшение, а исправление дефекта, который поймал тест + `TestUSwitchingServersSurvivesAConnectionThatSettlesOnRetry`: соединение может уйти через + close-callback сокета, который продолжает поколение и не делает takeover. Взводя сигнал только + на takeover, мы оставляли его завершённым после такого закрытия, и ожидающий крутился вхолостую + до собственного таймаута; +- будит единственная воронка состояний — `SetConnectionState`, и будит **до** дедупликации + уведомления и до потребительского обработчика: вопрос «стоит ли показывать событие» и вопрос + «изменилось ли состояние» — разные, и ожидающие не должны зависеть от первого. Плюс явное + пробуждение в `OnceOpen` **до** `OnConnected`: ожидающий ждёт сокет, а не обработчик; +- **и отдельное пробуждение после выходной бухгалтерии цикла переподключения.** Одной воронки + не хватает: признак, по которому ожидающий узнаёт исчерпание, — освобождённый источник + отмены, а он освобождается уже после уведомления. Разбуженный уведомлением ожидающий не видел + ничего терминального и парковался снова. По той же причине ожидающий спрашивает сначала про + поколение, которое сдалось (оно публикуется **до** уведомления), и только потом про источник — + иначе блокирующий обработчик статуса замыкал кольцо «обработчик ждёт ожидающего, ожидающий + ждёт бухгалтерию, бухгалтерия ждёт обработчика». + +**`ConnectionManager` всё равно чинится — отдельно от сигнала.** Отказавшись строить ожидание +на нём, оставить его как есть нельзя: `connectionManager` — публичное поле публичного типа +(`connection.cs:1323`), достижимое снаружи как +`client.connection.connectionManager.AwaitConnection()`, и соединение шлёт в него уведомления с +девяти точек, на своих потоках. Внутри SDK его никто не ждёт, поэтому дефекты не проявлялись — +но они не отсутствуют, а спят, и первый же потребитель, который вызовет `AwaitConnection()`, +встретит их все сразу. Проверено тестами до правки: ожидающий возобновлялся **внутри** +`ResolveAllAwaiting`, то есть внутри `OnceOpen` до `OnConnected`, а регистрация, пришедшаяся на +рассылку, роняла `InvalidOperationException` («Collection was modified») или теряла ожидающего +насовсем. Исправлено: список под блокировкой, снимок берётся под ней, а будятся ожидающие вне +её; `RunContinuationsAsynchronously`; `TrySet*` вместо `Set*`; отмена через `TrySetCanceled`. + +**Что осталось в `WaitForConnectionAsync` без изменений.** Все проверки: `IsConnected()`, +`CheckIfNotConnected()`, исчерпание бюджета, таймаут, токен. Сигнал говорит только «посмотри +ещё раз», решение принимают те же условия, что и раньше — поэтому наблюдаемое поведение +идентично опросу. Ушёл ровно `Task.Delay(100 ms)`, на его месте +`ready.WaitAsync(remaining, cancellationToken)`. + +**Результат.** Весь юнит-набор (1304 теста) зелёный в трёх прогонах подряд, и он же стал +быстрее: 43 секунды до правки, 30 после — на тестах соединения опрос был заметной долей времени. + +## 4. Решение, часть 3: терминальное уведомление называет причину + +Первая редакция добавляла `bool Exhausted` в `ReconnectInfo` и заполняла его на терминальном +уведомлении цикла. Это давало **два** правила чтения вместо одного: `Reconnect != null` +переставало значить «цикл работает», и различать приходилось по новому флагу. Признак стоит не +на том объекте. + +Вместо этого причина останова ставится на уведомление целиком: + +```csharp +public enum ConnectionStopReason +{ + None, // уведомление не терминальное + UserDisconnected, + ReconnectExhausted, + ConnectHandlerFailed, + InitialConnectionFailed, + ClosedPermanently, // соединение закрыто без переподключения +} + +public class ConnectionStatusInfo +{ + // ...существующие члены без изменений... + public ConnectionStopReason StopReason { get; set; } // умолчание None — аддитивно +} +``` + +Заполняется на всех десяти точках `Disconnected`: + +| Точки | `StopReason` | +|---|---| +| `:1792`, `:1803`, `:1856`, `:1866` | `UserDisconnected` | +| `:3202` | `ReconnectExhausted` | +| `:2774` | `ConnectHandlerFailed` | +| `:2350` | `InitialConnectionFailed` | +| `:2291`, `:3041`, `:3072` | `ClosedPermanently` | + +`ReconnectInfo` не меняется вовсе, `Reconnect` на терминальном уведомлении остаётся `null`, и +семантика `Reconnect != null` сохраняется. Этим же закрывается бывший открытый вопрос об отказе +на обработчике: на пути статуса он теперь называется, а не выводится из текста. + +## 5. Тесты + +**Написано 37, все зелёные.** Ниже — что именно они закрепляют; список отражает реализацию, а не +первоначальный замысел. + +`Tests/Xrpl.Tests/Client/Exceptions/TestUConnectionOutcomeTypes.cs` — 9 тестов о самих типах: +конструктор с причиной; все пять новых типов остаются `NotConnectedException`; `Attempts` равен +бюджету, а не сырому счётчику; `Failures` и `InnerException` у отказа обработчика; перекрытие +несёт вид и адрес; токена не несёт; подтип переживает прямой `await`; переживает `Task.WhenAll` +без сбойных задач; теряется рядом со сбойной. + +`Tests/Xrpl.Tests/Client/TestUConnectionManagerWaiters.cs` — 3 теста о примитиве ожидания: +ожидающий не возобновляется внутри уведомления; отмена даёт `Canceled`; регистрация во время +рассылки никого не теряет. + +`Tests/Xrpl.Tests/Client/TestUConnectionOutcomes.cs` — 25 тестов о том, какой путь даёт какой +ответ: + +| Что закрепляется | Тест | +|---|---| +| Подключения нет вовсе | `AClientThatNeverConnectedReportsThatNothingIsInProgress` | +| Бюджет переподключения исчерпан | `AWaiterLearnsTheReconnectBudgetWasSpent` | +| Пробуждение по факту отказа, а не по таймауту | `AWaiterIsWokenWhenTheClientGivesUpNotWhenItsOwnTimeoutExpires` | +| Отказ обработчика ≠ отключение потребителем | `GivingUpOnABrokenConnectHandlerSaysTheHandlerBroke` | +| Тот же ответ при обоих таймингах отказа | `ABrokenConnectHandlerAnswersTheSameWhicheverWayItFails` | +| Перекрытие другим `ChangeServer` | `AnOvertakenSwitchNamesTheSwitchThatWon` | +| То же на уровне `XrplClient` | `AnOvertakenSwitchIsReportedThroughTheClientToo` | +| Перекрытие **во время ожидания** (путь `:1153`) | `ASwitchOvertakenWhileWaitingReportsTheWinner` | +| Перекрытие `Connect` | `ASwitchAConnectOvertookNamesTheConnect` | +| Перекрытие `Disconnect` даёт «клиент выключен» | `ASwitchADisconnectOvertookIsToldTheClientIsDown` | +| Отказ запроса по `ImmediateFail` | `ARequestRefusedByPolicySaysSoRatherThanBlamingTheServer` | +| Свип запроса переключением | `ARequestSweptByASwitchNamesTheSwitch` | +| Свип запроса отключением | `ARequestSweptByADisconnectNamesTheDisconnect` | +| Свип запроса реконнектом | `ARequestSweptByAReconnectNamesTheReconnect` | +| Сетевой обрыв перехода не называет | `ARequestKilledByANetworkDropNamesNoTransition` | +| Повторный `Connect` запросы не трогает | `RedundantConnectDoesNotDisturbRequestsInFlight` | +| `StopReason` при исчерпании, `Reconnect` остаётся `null` | `TheTerminalNotificationSaysWhyTheClientStopped` | +| `StopReason` при отключении потребителем | `AConsumerDisconnectIsNamedInTheStatusStream` | +| `StopReason` при отказе обработчика и `None` на нетерминальных | `TheStatusStreamNamesABrokenHandlerAndOnlyWhenTerminal` | +| Исход как значение | `TheOutcomeOfAWaitIsAValueAndNamesTheCase` | +| Доступность через интерфейс и через класс | `TheOutcomeIsReachableThroughTheInterfaceAndTheClass` | +| Таймаут — ответ, отмена и плохой таймаут — исключения | `TimeoutIsAnAnswerAndCancellationIsNot` | +| Ожидающие не роняют друг друга | `WaitersDoNotTakeEachOtherDown` | +| `ChangeServer` переживает пересборку соединения | `SwitchingServersSurvivesAConnectionThatSettlesOnRetry` | +| Исчерпав бюджет, клиент остаётся остановленным — и не залипает | `AClientThatSpentItsReconnectBudgetStaysStopped` | + +Вспомогательные серверы, которых не хватало: `DropsFirstServerInfoServer` (роняет первый +`server_info` — единственное окно, где видно разницу в 2.7) и `SilentOnPingAndLedgerServer` +(молчит на `ping` и на запрос — позволяет держать запрос в полёте, пока health-check +пересобирает соединение). + +Отдельного теста «совместимость `catch`» нет: `Assert.IsInstanceOfType` и +`Helper.ThrowsExceptionAsync` принимают наследников, а существующие тесты +(`TestUConnectionTransitionOwner`, `TestUOnConnectedHandlerFailure`, +`TestURequestDuringServerSwitch`) уже ловят базовыми типами и потому сами и есть эта проверка — +ни один из них не потребовал правок. + +**Про гонку из 2.4.** Сценарий, который ей нужен, воспроизводил другой дефект — и тот теперь +исправлен здесь же (раздел 10), а «ровно одна серия» стала утверждением +`AClientThatSpentItsReconnectBudgetStaysStopped`. Отдельного теста на само сужение проверки +`:1339` нет: он требует попасть в зазор между отказом цикла и входом в ожидание, чего публичный +API детерминированно не даёт. Обоснование, почему сужать нельзя, остаётся в 2.4 и опирается на +достижимость `NotConnectingException` из `CheckIfNotConnected`. + +## 6. Расхождения с предложением + +Принято по существу. Отличия: + +1. **Типов не два, а пять** (раздел 1.1). `RequestRefusedException`, + `ConnectHandlerFailedException` и `NotConnectingException` в предложении отсутствуют, но + смешивают случаи, реакция на которые различна вплоть до противоположной: «узел не отвечает, + уходим» против «узел отвечает, отказал наш обработчик» против «вызови `Connect()`». +2. **`ConnectionSupersededException` несёт `Kind`, а не только `SupersededBy`**, и `Kind` + читается из состояния перехода, а не подставляется по месту. +3. **`bool` заменён перечислением** (раздел 3.1) — по той же причине, по которой предложение + отвергает таблицу соответствий. Предложенная форма `Task` не различала бы «сдался» и + «не успел». +4. **`WaitForConnectionAsync` потребителю формально доступен** — через + `IXrplClient.connection` (`IXrplClient.cs:85`), вопреки пункту 2 предложения. Проблема не в + недоступности, а в том, что путь идёт через внутренний объект. На `XrplClient` метода нет + вовсе — предложение считает, что есть. +5. **`HasConnectionAsync` не поднимается на интерфейс** (раздел 3.1), хотя предложение просило + именно `bool`-форму: перегрузка ломающая, а переопределение меняет поведение. +6. **Событийное ожидание сделано не донорским классом, а починкой своего** (раздел 3.2): + примитив в SDK уже был и уже был разослан по жизненному циклу — не хватало слушателя и + одного пробуждения. +7. **Причина ставится на `ConnectionStatusInfo`, а не на `ReconnectInfo`** (раздел 4) — сильнее + того, что просило предложение, и не ломает смысл `Reconnect != null`. + +Пункт 1 предложения (`Xrpl.Client.Exceptions.TimeoutException` на пути `ChangeServer`) +подтверждается полностью — см. 1.5. + +## 7. Что не меняется, и где граница + +- Поведение соединения. Ни один переход, ни один порядок, ни одно уведомление не меняет момента + или условия. Меняется только то, что можно прочитать о результате. +- Внутренняя проверка на `connection.cs:1339` остаётся широкой (2.4). +- `HasConnectionAsync` остаётся ровно такой, какая есть (3.1). +- Тексты сообщений исключений — дословно те же. +- `System.TimeoutException` на таймауте ожидания (`connection.cs:1406`) остаётся типом BCL: + это правильный тип, и подменять его своим значило бы добавить ловушку, а не убрать. +- `Xrpl.Client.Exceptions.TimeoutException` не трогаем. Ловушка «это не + `System.TimeoutException`» остаётся; она уже описана в XML-документации типа, а + переименование ломающее. +- **Свип, вызванный отказом соединения, остаётся отменой.** Сетевой обрыв и закрытие сокета + (`:2290`/`:2291`, `:2310`/`:2311`, `:2995`) отклоняют запросы через + `RejectAllWithCancellation`. Это решение, а не упущение: комментарий на `:2992` фиксирует + его — отмена выбрана, чтобы приложения-потребители не писали сетевой обрыв в Critical. + Причину такой остановки даёт `StopReason` на потоке статуса (раздел 4). Свипы, которые делает + переход, типизируются (2.6). +- `:2315`, `:2516`, `:2522` не трогаются: первый — мёртвый путь, второй и третий — остаточные + ветки (1.1). +- Базовые пакеты (`Xrpl.AddressCodec`, `Xrpl.BinaryCodec`, `Xrpl.Keypairs`) не затронуты и + версию не получают. + +## 8. Решения по бывшим открытым вопросам + +Открытых вопросов не осталось; ниже — что решено и почему, чтобы к этому не возвращались. + +1. **Свипы запросов в полёте — типизируются** (2.6). Все шесть точек, где свип делает переход, + отклоняют запросы `ConnectionSupersededException` с видом перехода. Тип наследует + `OperationCanceledException`, поэтому существующие `catch` и статусы задач не меняются, и + отдельного релиза это не требует. Свипы, вызванные отказом соединения, остаются отменой + намеренно (раздел 7). +2. **Готовый примитив ожидания со стороны потребителей не берётся** (3.2). Правовых + препятствий к этому не было, но и чужой код не нужен. Первоначально предполагалось построить + ожидание на `ConnectionManager`, который в SDK уже есть; от этого пришлось отказаться при + реализации — он будит ожидающих на отставке соединения, а отставка обязана переносить запрос + на новое соединение. Построен отдельный сигнал `_connectionReady`, а `ConnectionManager` + всё равно починен: он публичен и его дефекты живые (3.2). +3. **`SetNetworkId` на пути `ChangeServer` — защищается повторами здесь же** (2.7), вместе с + уточнением фильтра повторов под новые типы. Отдельной задачи не заводим: правка на две + строки, а её взаимодействие с `ConnectionSupersededException` всё равно проектируется в этой + же спеке, и разносить их по разным релизам значило бы проектировать дважды. + +## 9. Версия и changelog + +Аддитивно, с двумя оговорками, которые записаны честно, а не спрятаны: + +- новые типы наследуют бросаемым сегодня, поэтому ни один `catch` по базовому типу не меняет + смысла. Проверки на **точный** тип, фильтры `catch when`, сериализация исключений и + утверждения `ThrowsExactly` в чужих тестах — меняются. Это уточнение наследованием, а не + «всё аддитивно»; +- новые члены интерфейса имеют default-реализацию (главная библиотека таргетит + `net8.0;net9.0;net10.0`), но default-член виден только через ссылку типа интерфейса, поэтому + метод добавляется и в `XrplClient` (3.1). + +`ConnectionStopReason.None` — умолчание, `ReconnectInfo` не меняется, `HasConnectionAsync` не +меняется. + +**Внутренняя механика меняется в двух местах**, и оба наблюдаемого поведения не затрагивают: + +- свипы запросов отдают наследника `OperationCanceledException` вместо него самого (2.6) — ни + один `catch`, ни один статус задачи, ни один тест не меняется; +- ожидание готовности перестаёт опрашивать и начинает слушать отдельный сигнал + `_connectionReady` (3.2), который завершается из единственной воронки состояний и из выходной + бухгалтерии цикла переподключения. Это единственное место, где поведение строго улучшается: + ожидающий, который раньше досиживал до `ConnectionAcquisitionTimeout`, теперь узнаёт причину + сразу. + +Минорная версия: **11.5.0.0**, `` только в `Xrpl/Xrpl.csproj`. Один PR, один +релиз, возврата к этому коду не планируется. + +Запись в `CHANGES.md` — по образцу 11.4.0.0: какие типы читать вместо классификации по тексту, +что благодаря этому потребитель может удалить у себя (написанные вручную таблицы соответствий +«тип исключения → что случилось с соединением» и внешние флаги вида «это отключение моё»), и +где сознательная граница — свип, вызванный сетевым обрывом, остаётся отменой, а причину даёт +`StopReason`. + +## 10. `StopAfterMaxAttempts` действительно останавливает клиента + +Найдено при попытке написать тест 9 и сначала вынесено из объёма как посторонний дефект; по +решению пользователя разобрано и исправлено здесь же — это тот же жизненный цикл соединения, +что и вся работа. + +**Симптом.** Клиент подключён, пир закрывает сокет (код 1001), цикл переподключения тратит +бюджет и объявляет `Disconnected` / `ReconnectExhausted` — а следом стартует **вторая полная +серия** с попытки №1 и объявляет то же самое второй раз. + +**Не регрессия этой работы.** Проверено прогоном на неизменённом 11.4.0 (`f7040872`) в +отдельном worktree: последовательность уведомлений идентична. От health-check не зависит — +воспроизводится при `UseCustomPing = false`. + +**Корневая причина.** `StartReconnectLoop` отличает «серия уже идёт» по двум полям: +`_reconnectLoopGeneration` и `_reconnectCts`. Выходная бухгалтерия цикла обнуляет **оба** — то +есть оставляет ровно ту картину, которая означает «цикла нет». Закрытие сокета от последней +неудачной попытки приходит уже после этого, проходит проверку, а поскольку источник отмены +обнулён — попадает в ветку «свежая последовательность» и ставит `_reconnectAttempts` в ноль. +Факта «это поколение сдалось» не записывал никто: `_permanentlyDisconnected` ставит только +`Disconnect()`, а исчерпание бюджета — нет. + +**Исправление.** Поколение, чья серия исчерпала бюджет, запоминается +(`_reconnectExhaustedGeneration`) в ветке отказа, под `_transitionLock` и **до** уведомления — +по той же причине, по которой цикл запускают до уведомления: уведомление исполняет +потребительский код, и закрытие может лечь прямо в него. `StartReconnectLoop` отказывается +стартовать для такого поколения. + +Ключ — поколение, а не флаг, поэтому сбрасывать нечего: номера поколений только растут, и +`Connect()` или `ChangeServer` начинают новое — ровно тогда, когда спросить снова решил +потребитель, и это разрешено. + +**Закреплено** `TestUAClientThatSpentItsReconnectBudgetStaysStopped`: ровно одно терминальное +уведомление, и вторым утверждением — что остановленный клиент не залип: `ChangeServer` на живой +сервер подключается. Без правки тест падает на «Actual: 2». + +**Замечание о транспорте.** В браузере дефект не воспроизводился: там неудачная попытка кончается +`net_webstatus_ConnectFailure` без закрытия сокета, поэтому второго входа в close-callback нет. +Исправление от транспорта не зависит — оно на стороне решения о запуске серии. + +## 11. Холодное ревью: пять дефектов в новом коде + +Ветка прошла два прохода `cold-diff-review` — ревьюеры читают дифф без единого слова о замысле. +Первый проход дал три подтверждённых дефекта, второй — пять; все восемь в коде этого изменения, +ни одного унаследованного. Ниже — второй проход; первый описан в разделах 2.x и 10 по существу. + +### 11.1. Ожидание спрашивало не в том порядке + +`WaitForConnectionAsync` начиналось с `CheckIfNotConnected()`, и только потом заходило в цикл, где +проверяются терминальные состояния. После того как серия переподключений закончилась, у клиента +нет сокета, нет источника отмены и состояние `Disconnected` — то же самое, что у клиента, которому +`Connect()` ещё не вызывали. Ранняя проверка отвечала на это чтение первой. + +Наблюдаемо: поток статусов сообщает `ReconnectExhausted`, потребитель подтверждает это ожиданием +перед переключением на другой сервер — и получает `NotConnecting`, «вызовите `Connect()`» на том же +мёртвом адресе. Тот же клиент отвечал по-разному в зависимости лишь от того, стоял ли вызывающий в +ожидании до того, как цикл сдался. Исход, ради которого сделан весь этот API, оказался недоступен +с того места, откуда его естественно спрашивать. + +**Исправление.** «Ничего не происходит» — утверждение о моменте вызова, поэтому оно спрашивается +один раз (`firstPass`) и **последним** из четырёх терминалов, внутри того же критического участка. +Спрашивать его на каждом проходе нельзя: неудачная первая попытка объявляет `Disconnected` и лишь +затем запускает цикл, и в этом промежутке корректно ожидающий вызывающий получил бы отказ. + +**Закреплено** `TestUTheWaitAndTheStatusStreamAgreeAfterTheBudgetIsSpent` — утверждение делается +против потока статусов, а не против литерала: важно, что два способа спросить согласны. + +### 11.2. Постоянное закрытие узлом не заканчивало ожидание + +Коды 1002, 1003, 1007, 1010 — единственный путь, на котором соединение кончается без +`Disconnect()` потребителя и без серии переподключений. Ветка `OnceClose` объявляла +`ClosedPermanently` и не оставляла ожиданию ничего, по чему это опознать: сокета нет, цикла нет, а +счётчик попыток эта же ветка обнуляет. Стоявший в ожидании просыпался на уведомлении, не находил +ничего и парковался снова — чтобы через весь `ConnectionAcquisitionTimeout` (по умолчанию пять +минут) услышать «соединение не установлено вовремя» о соединении, про которое потребителю уже +сказали, что оно закрыто навсегда. + +**Исправление.** `_closedPermanentlyGeneration` пишется под `_transitionLock` до уведомления; +ожидание отвечает `ConnectionClosedPermanentlyException`, а `WaitForConnectionOutcomeAsync` — +`ConnectionWaitOutcome.ClosedPermanently`. `ConnectionStopReason.ClosedPermanently` получил +counterpart, которого у него не было. + +**Закреплено** `TestUAPermanentCloseEndsTheWaitInsteadOfRunningItOut` на новом сервере +`ClosesWithCodeServer`: существующие тестовые серверы такой кадр не отправляют. + +### 11.3. Ноль как «никакое поколение» + +Найдено собственным тестом при исправлении 11.1. Поколения считаются с нуля, первый переход +поднимает счётчик до единицы — значит ноль есть настоящее поколение клиента, которому ещё не +вызывали `Connect()`. И `_reconnectExhaustedGeneration`, и новое `_closedPermanentlyGeneration` +инициализировались нулём, поэтому сравнение `== _generation` на свежем клиенте истинно. Дефект +существовал и до этого прохода, но был скрыт ранней проверкой из 11.1. + +**Исправление.** Константа `NoGeneration = -1`. + +### 11.4. Одна проверка владения вместо трёх и одной пропущенной + +Объявление статуса, говорящее от имени одного перехода, истинно лишь пока этот переход владеет +соединением. Проверка стояла на местах вызова: у трёх была, у четвёртого — терминального +«переподключение остановлено» — не было. Пока дедупликация глушила повтор, это было невидимо; +`reasonChanged` (раздел 4) глушить перестал. + +Отдельно: проверка «серия закончена» в `OnConnectionFailed` бралась под `_transitionLock`, а +объявление шло уже после его отпускания — вызывающий мог прочитать «серия идёт», потерять +процессор, и опубликовать `RestoringConnection` после того, как цикл серию закончил и объявил об +этом. + +**Исправление.** `SetConnectionState` принимает `announcingGeneration`, и проверка владения вместе +с проверкой печати («это поколение уже объявлено остановленным») выполняется в том же критическом +участке, что и публикация состояния. Терминальное объявление печатью не глушится — оно и есть +печать. Вызывающие, которые говорят от имени клиента целиком, поколения не передают и не +проверяются. + +Что при этом **не** делается: строгий порядок доставки потребительских колбэков. Два конкурентных +объявителя могут вызвать обработчик в обратном порядке, и это свойство `SetConnectionState` в +базе — там метод не берёт замка вообще, ни на запись состояния, ни на дедупликацию, ни на вызов +потребителя. Гарантировать порядок можно только удерживая замок поперёк потребительского кода, а +это ровно тот дедлок, от которого метод защищается. + +### 11.5. Ожидание крутилось на границе таймаута + +Оба чтения часов квантованы системным тиком (15,625 мс на Windows), поэтому прошедшее время всегда +кратно тику, и таймаут, сам кратный тику, достигается точно, а не перешагивается: одна секунда — +64 тика, пять минут по умолчанию — 19200. Строгое сравнение `>` не срабатывало, а `remaining` +уже равнялось нулю, и охранник ниже отправлял цикл на новый проход, ничего не ожидая: спин на +`_transitionLock` соединения до конца тика, один раз на каждое истекающее ожидание. + +**Исправление.** Сравнение с дедлайном стало нестрогим; охранник `remaining <= Zero` оставлен и +теперь бросает таймаут, чтобы «оставшееся время не положительно» больше никогда не значило «пойти +на новый круг». + +**Закреплено** `TestUAWaitWhoseTimeoutLandsOnAClockTickStillTimesOut`. + +## 12. Третий проход: один учёт вместо россыпи + +Проход открыт по решению автора — условие скилла наступило (пасс 2 подтвердил high). Два +ревьюера, разные семейства, и оба нашли **одну и ту же дыру в разных местах**: терминальный статус +объявляется потребителю, но нигде не записывается, поэтому ожидание его не видит. Opus указал на +`InitialConnectionFailed` в `OnConnectionFailed`, codex — на `ClosedPermanently` в ветке +намеренного отключения того же метода. + +Это тот случай, о котором скилл говорит прямо: если находки растут, надо убирать механику, а не +добавлять охранники. Правки 11.2 и 11.4 добавляли по полю на каждое окончание — и следующее +окончание, у которого поля не завели, оказывалось невидимым ровно так же. + +### 12.1. Запись окончания переехала в узел объявления + +Введены `_stoppedGeneration` и `_stoppedReason`, которые пишет сам `SetConnectionState` — под тем +же замком, что публикует состояние, и только для объявления, которое действительно вышло наружу +(подавленное объявление не произошло). `_closedPermanentlyGeneration` удалён: он стал частным +случаем. + +Множество замкнуто по построению: терминальная причина не может дойти до потребителя, минуя +единственный метод, который её записывает. Поэтому поток статусов и ожидание не могут разойтись. + +`_reconnectExhaustedGeneration` остался рядом и не сливался: у него вторая работа — +`StartReconnectLoop` отказывает поколению, равному ему, — и он намеренно пишется **до** +объявления, чтобы закрытие, пришедшее во время потребительского кода, уже застало серию +законченной. + +`StoppedBecauseLocked()` — единственное место, переводящее `ConnectionStopReason` в тип, который +получит вызывающий. + +### 12.2. Путь запросов читает ту же запись + +`CheckIfNotConnected()` спрашивает `StoppedBecauseLocked()` перед своей широкой проверкой. +Потребитель, которому поток статусов только что сказал `ReconnectExhausted`, на запросе получал +`NotConnectingException` — «вы никогда не подключались», то есть ровно то различие, ради которого +заведено семейство исключений, опровергнутое третьим способом спросить. Проверка осталась широкой +(раздел 2.3): она лишь научилась отвечать точнее там, где ответ известен. + +### 12.3. Объявление отказа от сломанного обработчика получило поколение + +Четвёртое — и последнее — место, объявлявшее терминальную причину без `announcingGeneration`. +Проверка владения там read-only и без await, но захват с другого потока может лечь между ней и +объявлением, а дедупликация терминальные причины больше не глушит. + +### 12.4. Соответствие перечислений проверяется самим перечислением + +`TestUEveryStopReasonHasAnOutcomeOfTheSameName` перебирает `ConnectionStopReason` и требует +одноимённого `ConnectionWaitOutcome`. Тест сразу нашёл последнее расхождение: +`ConnectionStopReason.UserDisconnected` был сопоставлен с `ConnectionWaitOutcome.Disconnected` — +одно событие под двумя именами. Исход переименован в `UserDisconnected`. + +Добавлены `InitialConnectionFailedException` и `ConnectionWaitOutcome.InitialConnectionFailed`: +у причины, существовавшей с раздела 4, не было counterpart'а. + +Тест написан против перечисления, а не против списка, — поэтому причина, добавленная позже, будет +валить его, пока ей не заведут пару. + +## 13. Измеренное покрытие и удаление мёртвого значения + +Вопрос «покрыты ли тестами все варианты» был проверен перебором, а не памятью: по каждому значению +`ConnectionStopReason`, `ConnectionWaitOutcome`, каждому типу исключения и каждому +`ConnectionTransitionKind` посчитано, сколько раз оно утверждается в тестах. Три дыры: + +1. `ConnectionWaitOutcome.UserDisconnected` — 0 утверждений (исключение проверялось, значение нет). +2. `ConnectionWaitOutcome.ConnectHandlerFailed` — 0 утверждений (исключение проверялось 7 раз). +3. `ConnectionStopReason.InitialConnectionFailed` — 0 утверждений **везде**: ни причины, ни + исключения, ни исхода. + +Первые две закрыты утверждениями в существующих сценариях: значение-исход — отдельный путь +(исключение ловится и переводится), поэтому неверное сопоставление там не видно через исключение. + +### 13.1. Третья дыра оказалась не дырой в тестах, а мёртвым значением + +Причину нельзя было покрыть, потому что её ничто не порождает. Условие её появления — +`willReconnect == false`, то есть `wasOpen && !isNetworkDrop`. Но ветка, которая её называла, +достигается только для сокета, который не открылся как сессия, а такой сокет всегда +переподключают — это тот же факт, который читает `willReconnect`. Установившееся соединение с +11.4.0 сообщает о своём отказе через close-callback, а не сюда. + +Измерено, а не выведено. Дважды инструментировали и прогоняли весь набор: + +- метка на самой причине — **0 попаданий на 1328 тестах**; +- метка на `willReconnect == false` с выводом `wasOpen/drop/IsOpened/reconnectActive` — файл не + создан ни разу. + +Вторая метка заодно закрыла смежный вопрос: сочетание «объявили `RestoringConnection` и не +запустили цикл» невозможно по той же причине — оно управляется тем же `willReconnect`. + +### 13.2. Решение: удалить до релиза + +Удалены `ConnectionStopReason.InitialConnectionFailed`, `ConnectionWaitOutcome.InitialConnectionFailed` +и `InitialConnectionFailedException` (последние два были добавлены в разделе 12 именно под эту +причину). + +Значение, которое потребитель не может наблюдать, хуже отсутствующего: оно приглашает написать +ветку, которая никогда не исполнится. Добавить член перечисления позже — не ломающее изменение, +удалить позже — ломающее; поэтому ошибиться в эту сторону дешевле. + +Окончание, которое потребитель действительно получает для соединения, которое не поднялось и +перестало переподключаться, — `ReconnectExhausted`, от цикла, который перестал. + +Это же полностью снимает замечание CodeRabbit (Major) из раздела 4: причина не может быть названа +перед стартом переподключения, потому что причины больше нет. + +## 14. Отказ, сообщающий об успехе: найдено CI, не локальной машиной + +Юниты упали на CI, пройдя локально 1328/1328. Упало утверждение, добавленное в разделе 13, — +что значение-исход называет тот же случай, что исключение: + +``` +Failed TestUGivingUpOnABrokenConnectHandlerSaysTheHandlerBroke +Expected:. Actual:. +``` + +Локально не воспроизвелось за восемь прогонов. Но причина не в тайминге, а в порядке, который +гарантирован кодом, — просто окно узкое, и медленная машина в него попадает чаще. + +### 14.1. Корень + +Ветка отказа в `OnConnectHandlerFailedAsync` делает три шага подряд: + +1. объявляет окончание (`ConnectHandlerFailed`); +2. `requestManager.RejectAll(...)` — **это и резолвит вызывающего внутри `Connect()`**: он ждёт + `server_info`, который посылает `SetNetworkId` сразу после открытия сокета; +3. и только потом `Disconnect()`. + +Между 2 и 3 сокет ещё установлен в `ws` и открыт. `WaitForConnectionAsync` начинался с быстрой +проверки `if (IsConnected()) return;`, и внутри цикла `connected` проверялся раньше терминала. +Итого: потребителю только что сказали, почему клиент сдался, а на вопрос «подключён?» он получал +«да». Первая из форм отказа по списку холодного ревью — операция сообщает об успехе, когда +состояния, которое она описывает, уже нет. + +### 14.2. Правка + +- быстрая проверка на входе `WaitForConnectionAsync` убрана: цикл отвечает на тот же вопрос одним + замком позже, но со всем остальным состоянием в руках; +- `_permanentlyDisconnected` и `StoppedBecauseLocked()` спрашиваются **независимо** от того, открыт + ли сокет, и терминал бросается **раньше**, чем возвращается «подключено»; +- запись об окончании сбрасывается при `Connected` — иначе поколение, которое объявили + остановленным, а оно потом всё же подключилось (успевшая попытка после того, как цикл сдался), + вечно отвечало бы окончанием, пережившим состояние, которое оно описывает. + +### 14.3. Вторая половина, которую открыл тот же тест + +Детерминированный тест на это окно — `TestUAStoppedClientDoesNotCallItselfConnected` — использует +само терминальное уведомление как точку останова: оно объявляется до отклонения запросов, поэтому +внутри обработчика сокет гарантированно открыт, а окончание гарантированно записано. Стресс-тест +здесь был бы не нужен. + +Первый же его прогон показал, что «connected» действительно исчез, но ответ стал `UserDisconnected`. +`_connectHandlerGaveUp` писался только в `TakeOverLocked`, то есть на шаге 3, — поэтому в окне +`DisconnectedBecauseLocked` не знал причины и отвечал «клиента отключил потребитель». Ровно та +подмена, ради устранения которой затевалось всё изменение (раздел 2.2), просто в окне шириной в два +оператора. + +Причина теперь записывается **до** объявления, под `_transitionLock` и с проверкой владения, и тем +же значением, которое дальше получает `Disconnect()`, так что разойтись они не могут. + +### 14.4. Вывод про доверие к локальному прогону + +Тест, который проходит локально и падает на CI, — это не «флейк CI». Здесь локальный прогон +проходил по удаче: сокет обычно успевал закрыться. Дефект был детерминированным по построению, и +искать его надо было не в тайминге, а в порядке операций. + +## 15. Быстрый путь `Connect()` и граница тестируемости + +CodeRabbit на инкрементальном обзоре указал на следствие правки 14.2, которое я не заметил. +Находка помечена «Source: Learnings» — то есть выведена в том числе из его собственной памяти, — +поэтому проверялась по коду, а не принималась на веру. Проверка подтвердила: описано точно. + +### 15.1. Дефект + +`Connect()` начинался с быстрого выхода: + +```csharp +if (IsConnected()) +{ + SetConnectionState(XrpConnectionState.Connected, message: $"Already connected to {url}"); + return; +} +``` + +В окне из раздела 14 сокет открыт. Сообщение терминального уведомления кончается словами +«Call Connect() to retry», поэтому потребитель, делающий ровно это из обработчика статуса, — +ожидаемая реакция, а не экзотика. И он получал: объявление `Connected`, успешный возврат и +**ничего запущенного**. Соединение сносили через два оператора, а потребитель считал, что +переподключился. + +Дополнительно этот путь сбрасывал бы запись об окончании (правка 14.2), то есть портил ответ и +последующему ожидающему. + +### 15.2. Правка + +Быстрый выход спрашивает то же, что спрашивает ожидание, и под тем же замком: открыт ли сокет, +не отключён ли клиент навсегда и нет ли записанного окончания у текущего поколения. Иначе — +обычный путь подключения, то есть настоящий переход. + +Дефект шире, чем «портится запись»: `Connect()` возвращал успех для соединения, которое уже +кончилось. Это первая форма отказа из списка холодного ревью, в третий раз за эту ветку. + +### 15.3. Почему на это нет теста + +Детерминированного юнит-теста на это окно нет, и это записано как граница, а не как упущение. + +Окно открывается только внутри потребительского обработчика статуса, а этот обработчик +исполняется на потоке, который обслуживает сокет. Блокирующий `Connect()` оттуда не возвращается +никогда: новый handshake не может завершиться, пока поток занят его ожиданием. Две попытки +написать такой тест дали зависание на 10 и на 20 секунд соответственно, и обе были удалены — +висящий тест хуже отсутствующего. Асинхронный вариант (`Task.Run` из обработчика) детерминизма не +даёт: к моменту запуска сокет может успеть закрыться, и тогда тест проходит и на сломанном коде, +что хуже вдвойне. + +Что закреплено вместо этого: `TestUAStoppedClientDoesNotCallItselfConnected` доказывает на том же +окне, что решение — «записанное окончание старше открытого сокета» — принимается правильно. +`Connect()` теперь спрашивает ровно это решение, и то, что он его спрашивает, проверяется чтением +одной строки, а не тестом. + +Также стоит отметить, что потребитель, вызывающий `Connect().Wait()` из обработчика статуса, +повесит себя сам — по той же причине. Это свойство синхронного ожидания асинхронной операции на +потоке колбэка, а не этого изменения. + +## 16. Тот же класс в ещё четырёх местах + +Третья находка того же обзора пришла в свёрнутой секции «Outside diff range» — инлайном такие не +приходят, и поймать их можно только читая тело обзора целиком. + +Указано было одно место: `catch` цикла переподключения объявляет `RestoringConnection`, имея +проверку владения строкой выше, но не в том же критическом участке. Захват, легший между ними — +прежде всего пользовательский `Disconnect()`, — получал поверх своего состояния нестабильный +статус от цикла, который уже смещён. + +Прежде чем чинить точку, прошёлся по **всем** объявлениям и разметил, какие несут +`announcingGeneration`. Мест того же класса оказалось четыре: + +1. быстрое переподключение объявляет «Reconnecting immediately...» сразу после своего захвата; +2. оно же на отказе собственной попытки, сразу после `StartReconnectLoop(generation)`; +3. `OnConnectHandlerFailedAsync` в ветке «обработчик упал, но ещё не сдаёмся»; +4. `catch` цикла переподключения — то, что назвал обзор. + +Все четыре получили поколение. + +Утверждение из раздела 12.3 — что отказ обработчика был «четвёртым и последним» местом без +поколения — было верным ровно в том виде, в каком написано: речь шла о местах, объявляющих +**терминальную** причину. Эти четыре объявляют `RestoringConnection`, то есть не терминальны, и +под то утверждение не попадали. Но класс дефекта у них тот же, и разметка по всем местам сразу — +то, что надо было сделать в разделе 12, а не после третьего указания со стороны. + +Тогда я оставил без поколения три места — `Connecting` от захвата, пути `Disconnect()` и +`Connected` из `OnceOpen` — на том основании, что они «говорят от имени клиента целиком, а не +одного перехода». + +**Это рассуждение неверно, и оно опровергнуто в разделе 17.** Все три привязаны к поколению: +`TakeOverLocked` его назначает, а `ChangeServer`, оба пути отключения и `OnceOpen` дальше работают +с поколением своего перехода или своей сессии. Отдельные проверки `Owns` рядом с вызовом не делают +публикацию атомарной с захватом — для этого и нужен `announcingGeneration`. Абзац оставлен здесь +как след рассуждения, а не как действующее описание: читать надо раздел 17. + +## 17. Класс закрыт компилятором + +Обзор оспорил решение из раздела 16 — оставить три места без поколения, потому что они «говорят от +имени клиента целиком». Возражение было запрошено мной прямо, и оно оказалось верным. + +### 17.1. Почему рассуждение было неверным + +`Connecting` объявляет захват, который только что взял поколение. Это и есть один переход, а не +клиент целиком: если между захватом и объявлением ляжет другой захват, объявление станет чужим. + +Для `Disconnect()` последствие хуже подавления. Запись об окончании ключуется как +`announcingGeneration ?? _generation`, поэтому объявление без метки записывало причину против +поколения, **актуального на момент публикации**, то есть чужого. Устаревший `Disconnect()` ставил +`UserDisconnected` на поколение победившего захвата. + +Помечены все: `Connecting` у `ChangeServer` и у `Connect`, четыре объявления путей отключения, +`Connected` из `OnceOpen` и быстрый путь `Connect()` (там поколение снимается тем же замком, что и +сам ответ, а не читается в момент объявления). + +### 17.2. Исключение для `Connected` + +Печать исчерпанного поколения (12.1) не должна глушить `Connected`. Печать существует, чтобы +законченная серия не рапортовала о прогрессе, которого нет; соединение, которое реально +поднялось, — не «прогресс», а факт, опровергающий печать. Заглушить его значило бы оставить запись +стоять над живым клиентом: всем спрашивающим отвечают «серия сдалась», пока по сокету идёт трафик. + +### 17.3. Параметр стал обязательным + +После разметки объявлений без поколения не осталось ни одного. Значит, его можно потребовать +компилятором: `announcingGeneration` — обязательный параметр, третьим по счёту. + +Это единственное изменение за день, которое закрывает **класс**, а не экземпляр. За одну ветку этот +класс находили пять раз, каждый раз по одному месту, и каждый раз я закрывал найденное. Теперь +пропустить нельзя — только передать неверное поколение, а это ошибка меньшего размера и видная +глазом на месте вызова. + +Побочный эффект правки: 14 вызовов передавали `severity` позиционно и после сдвига перестали +компилироваться. Все переведены на именованный аргумент — компилятор нашёл их сам, что и было +смыслом упражнения.