diff --git a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor index e2eee217..0a213b84 100644 --- a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor +++ b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor @@ -397,7 +397,7 @@ await InvokeAsync(async () => { IsConnected = true; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentReconnectInfo = null; SessionStartTime = DateTime.Now; @@ -617,7 +617,7 @@ client.connection.OnTransaction += _onTransactionHandler; client.connection.OnLedgerClosed += _onLedgerClosedHandler; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentConnectionState = client.connection.CurrentConnectionState; await base.OnInitializedAsync(); @@ -627,7 +627,7 @@ if (client.connection.IsConnected()) { IsConnected = true; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentConnectionState = client.connection.CurrentConnectionState; StateHasChanged(); } diff --git a/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs b/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs new file mode 100644 index 00000000..f8f18fd3 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs @@ -0,0 +1,95 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Threading.Tasks; + +using Xrpl.Client; + +using XrplTests; + +namespace Xrpl.Tests +{ + /// + /// Boundary checks for the health-check timing options. Both feed a timer directly — + /// HealthCheckInterval is cast to an int of milliseconds on the WASM path, where zero + /// fires once and never repeats and out-of-range values are rejected by the timer itself — so a + /// bad value has to fail on the way in, naming the option, rather than quietly disabling the + /// check that recovers dead connections. + /// + [TestClass] + public class TestUHealthCheckOptions + { + private static XrplClient CreateClient(TimeSpan? healthCheckInterval = null, TimeSpan? inactivityTimeout = null) => + new XrplClient("ws://127.0.0.1:1", new XrplClient.ClientOptions + { + UseCustomPing = true, + UseCheckHealth = true, + HealthCheckInterval = healthCheckInterval ?? TimeSpan.FromSeconds(20), + InactivityTimeout = inactivityTimeout ?? TimeSpan.FromSeconds(60), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(1), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(1), + }); + + [TestMethod] + public void TestZeroHealthCheckIntervalIsRejected() + { + // Validation runs while the connection is being constructed, so the bad value is rejected + // before a client exists to connect with. + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.Zero)); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestNegativeHealthCheckIntervalIsRejected() + { + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.FromMilliseconds(-1))); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestOutOfRangeHealthCheckIntervalIsRejected() + { + // Past int.MaxValue milliseconds - the WASM timer cannot represent it + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.FromDays(30))); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestNonPositiveInactivityTimeoutIsRejected() + { + ArgumentException error = Helper.ThrowsException( + () => CreateClient(inactivityTimeout: TimeSpan.Zero)); + StringAssert.Contains(error.Message, "InactivityTimeout"); + } + + /// + /// The lower bound is 1ms, and the defaults must keep working — otherwise every existing + /// consumer would start failing on connect. + /// + [TestMethod] + public async Task TestBoundaryAndDefaultValuesAreAccepted() + { + // 1ms is the documented minimum: validation must let it through. Nothing is listening on + // port 1, so the connect attempt fails on the transport - not on config validation. + XrplClient atMinimum = CreateClient( + healthCheckInterval: TimeSpan.FromMilliseconds(1), + inactivityTimeout: TimeSpan.FromMilliseconds(1)); + + Exception minimumError = await Helper.ThrowsExceptionAsync(() => atMinimum.Connect()); + Assert.IsNotInstanceOfType( + minimumError, + typeof(ArgumentException), + $"1ms should pass validation, but connect failed with: {minimumError.Message}"); + + XrplClient atDefaults = CreateClient(); + Exception defaultError = await Helper.ThrowsExceptionAsync(() => atDefaults.Connect()); + Assert.IsNotInstanceOfType( + defaultError, + typeof(ArgumentException), + $"The default options should pass validation, but connect failed with: {defaultError.Message}"); + } + } +} diff --git a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs index c8c372e5..a47f8a46 100644 --- a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs +++ b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs @@ -298,8 +298,13 @@ public async Task TestRepeatedOnConnectedFailuresBackOff() } } - // ReconnectBaseDelay is 100ms and CalcBackoff doubles per attempt with 25% jitter, - // so the third gap is ~4x the first even at the extremes of the jitter range. + // CalcBackoff doubles per attempt off ReconnectBaseDelay (100ms), capped at + // ReconnectMaxDelay (1s), with 25% jitter. The handler-failure path seeds the counter + // with its consecutive-failure count, so the delays run 400ms, 800ms, then 1s (capped) + // — first to last is ~2.5x nominally, and still grows at the jitter extremes. + // This holds only while the configured cap stays above the earlier backoff values: with + // a cap at or below 400ms every gap would sit on the cap, and the comparison would come + // down to which way the jitter fell — a coin flip, not a stable result. // Comparing first vs last rather than each consecutive pair keeps the assertion // robust: what regressed before was a flat sequence, not the exact multiplier. Assert.IsTrue( diff --git a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs index 7d434a45..1ce93c07 100644 --- a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs +++ b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs @@ -265,6 +265,14 @@ public async Task TestFailedConnectDuringReconnectLeavesLoopRunning() } // The server appears. Nobody touches the client from here on. + // The port was handed out before the awaits above, so check it is still free. StartMock + // binds on this thread, so a port taken meanwhile would come out as a raw SocketException + // from the line below; this turns it into a statement of the actual cause. Diagnostics, + // not a fix: the check itself binds and releases, so the port can still be lost between + // here and StartMock. + Assert.IsTrue( + TestUtils.IsPortStillFree(laterPort), + $"Port {laterPort} was taken by another process while the test held it — rerun."); _secondRippled = StartMock(laterPort); DateTime deadline = DateTime.UtcNow + TimeSpan.FromSeconds(40); diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index 261bfdf0..84afbe80 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -212,6 +212,32 @@ public class ConnectionOptions /// public bool UseCheckHealth { get; set; } = false; + /// + /// Gets or sets how often the background health check runs — the timer that notices a socket + /// which is no longer Open and hands the client to the fast-reconnect path.
+ /// Default: 20 seconds, the interval this check has always used. + /// + /// + /// Exposed primarily so tests can exercise the ping and fast-reconnect paths without waiting + /// out the default interval; those paths were previously unreachable from a unit test, which + /// is why they went uncovered through several fixes. Lowering it in production only makes the + /// state check more frequent — it sends no network requests of its own. + /// + public TimeSpan HealthCheckInterval { get; set; } = TimeSpan.FromSeconds(20); + + /// + /// Gets or sets how long a connection may go without any inbound activity before the health + /// check treats it as dead and hands it to the fast-reconnect path.
+ /// Default: 60 seconds, the threshold this check has always used. + /// + /// + /// A socket whose peer vanished stays Open until the next I/O, so silence is the only + /// signal available without sending traffic. Exposed together with + /// so the fast-reconnect path is reachable from a test in + /// under a second instead of over a minute. + /// + public TimeSpan InactivityTimeout { get; set; } = TimeSpan.FromSeconds(60); + /// /// Gets or sets the policy that determines how failed requests are handled. /// @@ -237,6 +263,23 @@ private void ValidateConfig() throw new ArgumentException( $"ConnectionAcquisitionTimeout ({config.ConnectionAcquisitionTimeout.TotalSeconds}s) must be >= ConnectionAttemptTimeout ({config.ConnectionAttemptTimeout.TotalSeconds}s) to allow at least one full connection attempt."); } + + // The WASM timer takes this as an int of milliseconds: zero fires once and never repeats, + // and anything past int.MaxValue or below zero is rejected outright by the timer itself. + // Fail here instead, where the message can say which option is wrong. + double healthCheckMs = config.HealthCheckInterval.TotalMilliseconds; + if (healthCheckMs < 1 || healthCheckMs > int.MaxValue) + { + throw new ArgumentException( + $"HealthCheckInterval ({config.HealthCheckInterval}) must be between 1ms and {int.MaxValue}ms."); + } + + if (config.InactivityTimeout <= TimeSpan.Zero) + { + throw new ArgumentException( + $"InactivityTimeout ({config.InactivityTimeout}) must be positive - a non-positive value would " + + "treat every connection as dead on the first health check."); + } } // https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/src/client/connection.ts createWebSocket @@ -296,9 +339,11 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con /// /// /// Not every touch of these fields is covered: the per-iteration _reconnectAttempts++ in - /// , and the plain resets in ChangeServer and - /// OnceClose, still run outside it. Those predate this lock; do not read the list above - /// as "all three fields are always synchronized". + /// , the plain resets in ChangeServer and + /// OnceClose, and the "is a loop already running" pre-checks in + /// OnConnectionFailed and OnceClose (which read _reconnectLoop, a + /// non-volatile field, outside the lock) all still run outside it. Those predate this lock; do + /// not read the list above as "all three fields are always synchronized". /// /// /// @@ -405,14 +450,27 @@ private void SetConnectionState( _previousNotifiedMessage = message; - OnConnectionStatus?.Invoke( - new ConnectionStatusInfo - { - Message = message, - Severity = severity, - Reconnect = reconnect, - ConnectionState = newState, - }); + // 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 + // costs the client its reconnect: the fast-reconnect path (running on a ping task whose + // callers swallow everything) and ReconnectLoopAsync, which notifies before its first + // connection attempt and would fault with _reconnectCts still installed and no live loop. + // A consumer's status handler must not be able to take the connection down. + try + { + OnConnectionStatus?.Invoke( + new ConnectionStatusInfo + { + Message = message, + Severity = severity, + Reconnect = reconnect, + ConnectionState = newState, + }); + } + catch (Exception notifyError) + { + Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw for state {newState}: {notifyError.Message}"); + } } private ReconnectInfo BuildReconnectInfo(int? explicitAttempt = null, TimeSpan? delay = null) @@ -572,18 +630,22 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // so a concurrent stop/start cannot dispose the source created here. Cancellation and // disposal of the old source happen after the lock is released. CancellationTokenSource oldCts; + CancellationTokenSource ownCts; lock (_reconnectStateLock) { oldCts = _reconnectCts; _reconnectLoop = null; // Clear old loop reference so StartReconnectLoop can start a new one _reconnectAttempts = 1; - _reconnectCts = new CancellationTokenSource(); + ownCts = new CancellationTokenSource(); + _reconnectCts = ownCts; } oldCts?.Cancel(); oldCts?.Dispose(); // 4. Now send first notification - IsReconnectActive() will return true + // Consumer handler exceptions are contained inside SetConnectionState - an escaping throw + // here would leave the source installed above with no loop and nobody to dispose it. SetConnectionState( XrpConnectionState.RestoringConnection, message: $"{reason} Reconnecting immediately...", @@ -646,7 +708,34 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) _pingTimeoutSocket = null; _networkDropSocket = null; - // 11. Reset permanentlyDisconnected for new connection + // 11. Reset permanentlyDisconnected for new connection - unless the user asked to disconnect + // while the awaits above were running. Disconnect() sets the flag, clears the reconnect + // state and then waits on the ping task this method runs inside, so it is still blocked + // here and cannot have finished its teardown. Clearing its flag and reconnecting anyway + // would resurrect a client the consumer explicitly took down - and Disconnect() would + // return reporting success while a fresh session was being built behind it. + if (_permanentlyDisconnected) + { + CancellationTokenSource abandoned = null; + lock (_reconnectStateLock) + { + if (ReferenceEquals(_reconnectCts, ownCts)) + { + abandoned = ownCts; + _reconnectCts = null; + _reconnectAttempts = 0; + _reconnectLoop = null; + } + } + + abandoned?.Cancel(); + abandoned?.Dispose(); + _isFastReconnectActive = false; + + Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned before connecting - the client was disconnected by the user"); + return; + } + _permanentlyDisconnected = false; // Note: _reconnectAttempts and _reconnectCts already set at the start of this method @@ -657,19 +746,41 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // and Connecting would overwrite ReconnectInfo, confusing consuming apps try { - await ConnectInternalAsync().ConfigureAwait(false); - await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, CancellationToken.None).ConfigureAwait(false); + // Pass the token of the session this method owns: a user Disconnect() cancels it, so the + // attempt below stops instead of opening a socket behind a client that was taken down. + // Disconnect() waits only briefly for the ping task, while acquisition can run much + // longer, so the flag check above cannot cover this window on its own. + await ConnectInternalAsync(ownCts.Token).ConfigureAwait(false); + await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, ownCts.Token).ConfigureAwait(false); // Connect succeeded - cleanup reconnect state // Note: _reconnectMode will be cleared in OnceOpen when connection is fully established _isFastReconnectActive = false; - CancellationTokenSource settled; + // Only tear down the source this method installed. The awaits above give a concurrent + // path (RestartReconnectLoop from a failing OnConnected handler, say) room to install a + // newer one; cancelling and disposing that would strand the sequence it belongs to, + // which is the same wedge the ownership checks in ReconnectLoopAsync guard against. + // When ownership is lost, ownCts needs no cleanup here: whoever evicted it from the + // field cancelled and disposed it as part of doing so. + CancellationTokenSource settled = null; lock (_reconnectStateLock) { - settled = _reconnectCts; - _reconnectCts = null; - _reconnectAttempts = 0; + if (ReferenceEquals(_reconnectCts, ownCts)) + { + settled = ownCts; + _reconnectCts = null; + _reconnectAttempts = 0; + + // Drop the task reference in the same transaction, for the same reason + // StopReconnectLoop does: a loop may have been started on this very source + // while the awaits above were running (OnConnectionFailed sees no live loop - + // the entry above cleared the reference - and StartReconnectLoop reuses a + // still-valid source). Cancelling that source without clearing the reference + // leaves every loopIsRunning check looking at a task that is exiting, so + // nobody starts a replacement and nobody reconnects. + _reconnectLoop = null; + } } settled?.Cancel(); @@ -680,13 +791,33 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // If Connect fails, transition to loop reconnect mode // Keep _reconnectMode set (will be LoopReconnect after StartReconnectLoop) - // _reconnectCts is already set, so StartReconnectLoop will reuse it + // A user Disconnect() can land while the awaits above are running - and it will wait on + // the very ping task this method runs inside, so it cannot have finished yet. Handing + // the client back to a reconnect loop then would undo an explicit disconnect. The flag + // is the authority: leave the state alone and let Disconnect() finish its teardown. + if (_permanentlyDisconnected) + { + Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned - the client was disconnected by the user: {ex.Message}"); + return; + } + + // Start the loop BEFORE notifying: SetConnectionState calls into consumer code, and an + // exception from a handler must not cost us the reconnect loop. Ordering matters more + // than the message here - without the loop the client never comes back. + // + // StartReconnectLoop reuses the source installed above when it is still there. It may + // not be: the awaits could have let another path replace or clear it, the same way the + // success branch above can no longer assume it still owns ownCts. Either outcome is + // survivable here - a live foreign loop makes the call return early, a cleared source + // makes it start a fresh sequence (losing only the seeded first delay) - so this path + // does not need an ownership check of its own. + StartReconnectLoop(); + SetConnectionState( XrpConnectionState.RestoringConnection, message: $"Reconnection failed: {ex.Message}. Retrying...", ConnectionCloseSeverity.Warning, reconnect: BuildReconnectInfo()); - StartReconnectLoop(); } } @@ -2050,14 +2181,15 @@ private void ClearReconnectState() _reconnectMode = ReconnectMode.None; } - /// - /// Value to seed _reconnectAttempts with when a fresh reconnect sequence starts. - /// Defaults to 0 — a genuine new sequence begins at the base delay. The OnConnected-handler - /// path passes its own consecutive-failure count instead: that path tears the loop down and - /// starts it again on every failure, so with a 0 seed the backoff would restart at - /// ReconnectBaseDelay each time and never grow. - /// - private void StartReconnectLoop(int initialAttempts = 0) + /// + /// Starts a reconnect loop unless one is already running, reusing a pre-created cancellation + /// source when there is one. A fresh sequence starts its attempt counter at zero, so the first + /// delay is CalcBackoff(1) — twice ReconnectBaseDelay — except on the + /// ping-timeout and network-drop paths, where the first attempt skips the delay entirely. The + /// OnConnected-handler path needs a seeded counter instead and uses + /// . + /// + private void StartReconnectLoop() { // Set reconnect mode to LoopReconnect (upgrades from FastReconnect or sets from None) _reconnectMode = ReconnectMode.LoopReconnect; @@ -2091,7 +2223,7 @@ private void StartReconnectLoop(int initialAttempts = 0) // Retire the old CTS after the lock is released - see _reconnectStateLock retired = existingCts; _reconnectCts = new CancellationTokenSource(); - _reconnectAttempts = initialAttempts; + _reconnectAttempts = 0; } // else: Reuse existing valid CTS (pre-created for fast reconnect) // Don't reset _reconnectAttempts - this is continuation of existing reconnect sequence @@ -2333,8 +2465,8 @@ private void StartWasmPingTimer(CancellationTokenSource cts) _ = ExecutePingCheckAndReleaseAsync(innerCts, tcs); }, state: cts, - dueTime: 20000, - period: 20000); + dueTime: (int)config.HealthCheckInterval.TotalMilliseconds, + period: (int)config.HealthCheckInterval.TotalMilliseconds); } private async Task ExecutePingCheckAndReleaseAsync(CancellationTokenSource cts, TaskCompletionSource tcs) @@ -2403,11 +2535,13 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts) return; } - if (timeSinceLastActivity > 60) + double inactivityLimit = config.InactivityTimeout.TotalSeconds; + if (timeSinceLastActivity > inactivityLimit) { _pingTimeoutSocket = ws; - await RetireCurrentSessionAndReconnectAsync("Connection timeout (no activity for 60+ seconds)."); + await RetireCurrentSessionAndReconnectAsync( + $"Connection timeout (no activity for {inactivityLimit:F0}+ seconds)."); return; } @@ -2518,7 +2652,7 @@ private void StartPingTimer() } else { - pingTimer = new Timer(20000); + pingTimer = new Timer(config.HealthCheckInterval.TotalMilliseconds); pingTimer.Elapsed += (sender, e) => { if (cts.IsCancellationRequested)