diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledAcceptLoop.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledAcceptLoop.cs new file mode 100644 index 00000000..8cabf8f2 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledAcceptLoop.cs @@ -0,0 +1,194 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Sockets; +using System.Threading.Tasks; + +using Xrpl.Client; + +namespace Xrpl.Tests +{ + /// + /// Guards the mock server's accept loop against a single bad connection taking it down. + /// + /// + /// The mock re-arms BeginAccept as the last statement of its accept callback, so any + /// throw earlier in that callback — a peer that resets the connection before or during the + /// WebSocket handshake — used to end the loop for good. The listen socket stayed bound, so the + /// port still looked taken and connects still completed at the TCP level, but nothing was ever + /// accepted again: every later client hung until its own connect timeout. + /// + /// That is not a hypothetical. It is what made flaky + /// on CI: concurrent ChangeServer/Disconnect calls abort half-open connections, one of those + /// aborts silenced the mock, and the assertion at the end of the test then blamed the client + /// for not reaching "a server that is up" — while the server was in fact deaf. + /// + [TestClass] + public class TestUMockRippledAcceptLoop + { + private CreateMockRippled _mock; + private int _port; + + 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" }, + } + }, + } + }, + }; + + [TestInitialize] + public void MyTestInitialize() + { + _port = TestUtils.GetFreePort(); + _mock = new CreateMockRippled(_port) { suppressOutput = true }; + _mock.AddResponse("server_info", ServerInfoResponse()); + _mock.Start(); + } + + [TestCleanup] + public void MyTestCleanup() => _mock?.Stop(); + + /// + /// Resets a connection while the mock is in its accept callback, then requires the mock to + /// still serve the next client. + /// + [TestMethod] + public async Task TestAbortedHandshakeLeavesTheMockAccepting() + { + // A zero linger time makes Close() send RST rather than FIN, so the mock's blocking + // Receive of the handshake fails with "connection reset by peer" — the exact throw + // seen in the CI log of the flaky run. + using (Socket rude = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) + { + rude.LingerState = new LingerOption(true, 0); + await rude.ConnectAsync(IPAddress.Loopback, _port); + rude.Close(); + } + + // Give the mock a moment to run its callback and (before the fix) fall out of it. + await Task.Delay(TimeSpan.FromMilliseconds(200)); + + XrplClient client = new XrplClient($"ws://127.0.0.1:{_port}", new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(5), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(3), + UseCustomPing = false, + }); + + try + { + await client.Connect(); + + Assert.IsTrue( + client.connection.IsConnected(), + "The mock stopped accepting after one aborted connection — its accept loop was not re-armed."); + } + finally + { + try + { + await client.Disconnect(); + } + catch (Exception) + { + // Cleanup must not mask the assertion above. + } + } + } + + /// + /// The handshake probe the reconnect tests use to attribute a failure must actually tell + /// a serving mock from a deaf one — a probe that always says "alive" would be worse than + /// none, since it would confirm the wrong suspect. + /// + [TestMethod] + public void TestHandshakeProbeTellsAServingMockFromADeafOne() + { + TimeSpan timeout = TimeSpan.FromSeconds(2); + + Assert.IsTrue( + TestUtils.MockCompletesHandshake(_port, timeout), + "A running mock must answer the probe with a 101 upgrade."); + + Assert.IsFalse( + TestUtils.MockCompletesHandshake(TestUtils.GetFreePort(), timeout), + "Nothing listens on that port, so the probe must report it as not serving."); + + // The case the probe exists for: a socket that is bound and listening but never + // accepts. The TCP connect still succeeds — which is why a plain connect check proves + // nothing — and only the missing handshake reveals that the server is deaf. + TcpListener deaf = new TcpListener(IPAddress.Loopback, 0); + deaf.Start(); + try + { + int deafPort = ((IPEndPoint)deaf.LocalEndpoint).Port; + Assert.IsFalse( + TestUtils.MockCompletesHandshake(deafPort, timeout), + "A listening socket that never accepts must be reported as not serving."); + } + finally + { + deaf.Stop(); + } + } + + /// + /// The same guarantee under repetition: a run of aborted connections must not degrade the + /// mock, since the reconnect tests abort several in a row. + /// + [TestMethod] + public async Task TestRepeatedAbortedHandshakesLeaveTheMockAccepting() + { + for (int i = 0; i < 10; i++) + { + using Socket rude = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + rude.LingerState = new LingerOption(true, 0); + await rude.ConnectAsync(IPAddress.Loopback, _port); + rude.Close(); + } + + await Task.Delay(TimeSpan.FromMilliseconds(200)); + + XrplClient client = new XrplClient($"ws://127.0.0.1:{_port}", new XrplClient.ClientOptions + { + RequestPolicy = RequestFailurePolicy.ImmediateFail, + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(5), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(3), + UseCustomPing = false, + }); + + try + { + await client.Connect(); + + Assert.IsTrue( + client.connection.IsConnected(), + "The mock stopped accepting after a run of aborted connections."); + } + finally + { + try + { + await client.Disconnect(); + } + catch (Exception) + { + // Cleanup must not mask the assertion above. + } + } + } + } +} diff --git a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs index 1ce93c07..56a8e4ed 100644 --- a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs +++ b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs @@ -1,4 +1,4 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.VisualStudio.TestTools.UnitTesting; using System; using System.Collections.Generic; @@ -161,10 +161,17 @@ public async Task TestConcurrentChangeServerKeepsClientRecoverable() await Task.Delay(TimeSpan.FromMilliseconds(100)); } - Assert.IsTrue( - _client.connection.IsConnected(), - "After concurrent ChangeServer calls the client could not reach a server that is up — " + - "the reconnect session was left disposed or orphaned."); + if (!_client.connection.IsConnected()) + { + // Say which side failed. A mock whose accept loop died still holds the port, so + // "the server is up" is an assumption worth checking before blaming the client — + // this test spent a CI run being read as a client bug for exactly that reason. + bool mockServing = TestUtils.MockCompletesHandshake(_port, TimeSpan.FromSeconds(2)); + Assert.Fail( + "After concurrent ChangeServer calls the client could not reach a server that is up — " + + "the reconnect session was left disposed or orphaned. " + + $"(mock still completes a handshake: {mockServing})"); + } } /// diff --git a/Tests/Xrpl.Tests/MockRippled/Server.cs b/Tests/Xrpl.Tests/MockRippled/Server.cs index 4f8fdef5..9af307f5 100644 --- a/Tests/Xrpl.Tests/MockRippled/Server.cs +++ b/Tests/Xrpl.Tests/MockRippled/Server.cs @@ -254,10 +254,14 @@ public void Stop() /// The async operation state private void connectionCallback(IAsyncResult AsyncResult) { + // Held until ownership passes to the MockClient, so a handshake that throws + // half-way closes the socket instead of leaking it for the process lifetime. + Socket clientSocket = null; + try { // Gets the client thats trying to connect to the server - Socket clientSocket = GetSocket().EndAccept(AsyncResult); + clientSocket = GetSocket().EndAccept(AsyncResult); // Read the handshake updgrade request byte[] handshakeBuffer = new byte[1024]; @@ -267,25 +271,55 @@ private void connectionCallback(IAsyncResult AsyncResult) string requestKey = Helpers.GetHandshakeRequestKey(Encoding.Default.GetString(handshakeBuffer)); string hanshakeResponse = Helpers.GetHandshakeResponse(Helpers.HashKey(requestKey)); - // Send the handshake updgrade response to the connecting client + // Send the handshake updgrade response to the connecting client clientSocket.Send(Encoding.Default.GetBytes(hanshakeResponse)); - // Create a new client object and add + // Create a new client object and add // it to the list of connected clients MockClient client = new MockClient(this, clientSocket); + clientSocket = null; _clients.Add(client); - // Call the event when a client has connected to the listen server + // Call the event when a client has connected to the listen server if (OnClientConnected == null) throw new Exception("Server error: event OnClientConnected is not bound!"); OnClientConnected(this, new OnClientConnectedHandler(client)); + } + catch (ObjectDisposedException) + { + // Stop() closed the listen socket: nothing left to accept on, and re-arming + // below would only throw again. + return; + } + catch (Exception Exception) + { + Debug.WriteLine("An error has occured while trying to accept a connecting client.\n\n{0}", Exception.Message); - // Start to accept incomming connections again - GetSocket().BeginAccept(connectionCallback, null); + try + { + clientSocket?.Close(); + } + catch (Exception) + { + // The peer is already gone; nothing to salvage. + } + } + // Re-arm unconditionally. This call used to be the last statement of the try block, + // so a peer that reset the connection during the handshake ended the accept loop for + // good: the listen socket stayed bound — the port still looked taken and TCP connects + // still completed — while nothing was ever accepted again, and every later client hung + // until its own connect timeout. One bad connection must not deafen the mock. + try + { + GetSocket().BeginAccept(connectionCallback, null); + } + catch (ObjectDisposedException) + { + // Stop() ran while this callback was in flight. } catch (Exception Exception) { - Debug.WriteLine("An error has occured while trying to accept a connecting client.\n\n{0}", Exception.Message); + Debug.WriteLine("An error has occured while re-arming the accept loop.\n\n{0}", Exception.Message); } } diff --git a/Tests/Xrpl.Tests/TestUtils.cs b/Tests/Xrpl.Tests/TestUtils.cs index 746e7d2c..a0fdf567 100644 --- a/Tests/Xrpl.Tests/TestUtils.cs +++ b/Tests/Xrpl.Tests/TestUtils.cs @@ -1,10 +1,11 @@ - + // https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/test/testUtils.ts using System; using System.Collections.Concurrent; using System.Net; using System.Net.Sockets; +using System.Text; namespace Xrpl.Tests { @@ -48,6 +49,50 @@ static public int GetFreePort() "GetFreePort: could not obtain an unclaimed loopback port after 50 attempts"); } + /// + /// Whether a mock server on still completes a WebSocket handshake. + /// + /// + /// A dead accept loop leaves the listen socket bound, so a plain TCP connect still + /// succeeds and proves nothing — only an answered handshake shows the mock is serving. + /// Tests use this to say whether a connection failure was the client's doing or the + /// mock's, instead of blaming the client for a server that went deaf. + /// + static public bool MockCompletesHandshake(int port, TimeSpan timeout) + { + try + { + using TcpClient probe = new TcpClient(); + if (!probe.ConnectAsync(IPAddress.Loopback, port).Wait(timeout)) + { + return false; + } + + NetworkStream stream = probe.GetStream(); + stream.WriteTimeout = (int)timeout.TotalMilliseconds; + stream.ReadTimeout = (int)timeout.TotalMilliseconds; + + // The mock reads the key by offset from "Sec-WebSocket-Key: ", so the header must + // carry a full 24-character value like a real client sends. + byte[] request = Encoding.ASCII.GetBytes( + "GET / HTTP/1.1\r\n" + + "Host: 127.0.0.1\r\n" + + "Upgrade: websocket\r\n" + + "Connection: Upgrade\r\n" + + "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" + + "Sec-WebSocket-Version: 13\r\n\r\n"); + stream.Write(request, 0, request.Length); + + byte[] buffer = new byte[256]; + int read = stream.Read(buffer, 0, buffer.Length); + return read > 0 && Encoding.ASCII.GetString(buffer, 0, read).Contains("101"); + } + catch (Exception) + { + return false; + } + } + /// /// Whether can still be bound on loopback right now. Tests that /// hold a port across an await use this to fail fast with a clear reason instead of