From 54d09f3b34538387acbecde9bd4dabd3453af787 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Wed, 26 Aug 2026 09:58:21 -0300 Subject: [PATCH 1/3] test(mock): the mock server stops taking the test host down with it A run on #145 died with "Server error: OnClientDisconnected is not bound!" and "Test host process crashed". The chain, in order: Server's constructor called start(), so the socket accepted connections before the caller had bound a single handler. CreateMockRippled.Start() binds after constructing, OnClientDisconnected last of the four, leaving a window where the server was live and had no subscribers. MockClient.messageCallback is a socket callback, so it runs on a thread-pool thread. When the socket faults it enters its own catch, and that catch calls ClientDisconnect - which threw when nothing was subscribed. An exception raised inside a catch block on a pool thread has nowhere left to go, so the runtime ends the process. That last part is what makes this worth more than a rerun. An aborted run does not report the tests it never reached: CI shows one failed job where in truth an unknown number of tests did not execute. The failure hides its own size, and the only thing that saved it from being silent is the non-zero exit code. Three changes, each closing one link: Events no longer throw when unsubscribed - all four now use ?.Invoke. For an event, having no subscriber is a legitimate state, not a server fault, and these fire from threads where a throw is not a failed assertion. Handlers are bound before the socket accepts. The constructor no longer listens; StartListening() is explicit, and CreateMockRippled calls it inside the same lock that guards Stop(), so nothing slips between publishing the server and it beginning to accept. This closes the window rather than merely surviving it: a request arriving before OnMessageReceived was bound used to go unanswered, which a test sees as a timeout rather than as a race. _clients is guarded. It was mutated from accept and disconnect callbacks - both pool threads - and read from the test thread with no synchronisation. An add during an enumeration throws InvalidOperationException on a thread with no catch above it: the same fatal shape by a different route. Four tests pin the invariants. Restoring the throw in ClientDisconnect fails two of them. Six consecutive local runs of the unit suite are clean, though that is weak evidence about a rare race - the argument is the mechanism, not the sample. No CHANGES.md entry: this is test infrastructure with no consumer-visible effect, the same call made for the test-only #139. --- .../Client/TestUMockRippledServer.cs | 162 ++++++++++++++++++ Tests/Xrpl.Tests/CreateMockRippled.cs | 32 ++-- Tests/Xrpl.Tests/MockRippled/Server.cs | 83 ++++++--- 3 files changed, 241 insertions(+), 36 deletions(-) create mode 100644 Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs new file mode 100644 index 00000000..013cd95f --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs @@ -0,0 +1,162 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Threading.Tasks; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Xrpl.Tests.MockRippled; + +namespace XrplTests.Xrpl.ClientLib; + +/// +/// The mock rippled server's own invariants - the ones whose absence took the whole test host +/// down rather than failing a test. +/// +/// +/// +/// A run on PR #145 aborted with Server error: OnClientDisconnected is not bound! and +/// Test host process crashed. The chain: MockClient.messageCallback is a socket +/// callback, so it runs on a thread-pool thread; when the socket faults it enters its own +/// catch, and from inside that catch it calls ClientDisconnect, which threw when +/// nothing was subscribed. An exception raised inside a catch block on a pool thread has nowhere +/// left to go, and .NET ends the process. +/// +/// +/// Worth testing rather than just fixing, because of how the failure presents: the run is +/// aborted, so the tests that had not been reached yet never run, and CI reports one failed job +/// rather than a few hundred unexecuted tests. It is a failure that hides its own size, and the +/// only reason it was not worse is that the process exits non-zero. +/// +/// +[TestClass] +public class TestUMockRippledServer +{ + private static IPEndPoint AnyLoopbackPort() => new IPEndPoint(IPAddress.Loopback, 0); + + /// + /// Raising an event nobody subscribed to is not an error. + /// + /// + /// All four events used to throw when unbound. For an event, no subscriber is a legitimate + /// state - and these fire from socket callbacks, where the difference between throwing and + /// not is the difference between a failed test and no test results at all. + /// + [TestMethod] + public void TestUAnUnsubscribedEventIsNotAnError() + { + Server server = new Server(AnyLoopbackPort()); + + try + { + // The one that actually crashed the host, called exactly as the catch block calls it. + server.ClientDisconnect(null); + + // And the others, which sit on the same kind of thread. + server.ReceiveMessage(null, "{}"); + } + finally + { + server.Stop(); + } + } + + /// + /// The client list survives being touched from several threads at once. + /// + /// + /// _clients is added to from the accept callback and removed from on disconnect - both + /// thread-pool threads - while the test thread reads it through GetConnectedClient and + /// GetConnectedClientCount. Unsynchronised, an add during an enumeration throws + /// on a thread with no catch above it, which is the + /// same fatal shape as the bug above by a different route. + /// + [TestMethod] + public async Task TestUTheClientListToleratesConcurrentUse() + { + Server server = new Server(AnyLoopbackPort()); + + try + { + List workers = new List(); + + for (int i = 0; i < 4; i++) + { + workers.Add(Task.Run(() => + { + for (int n = 0; n < 2_000; n++) + { + server.ClientDisconnect(null); + } + })); + + workers.Add(Task.Run(() => + { + for (int n = 0; n < 2_000; n++) + { + server.GetConnectedClientCount(); + server.GetConnectedClient(0); + server.GetConnectedClient("no-such-guid"); + } + })); + } + + await Task.WhenAll(workers); + } + finally + { + server.Stop(); + } + } + + /// + /// A server that has not been told to listen is not listening. + /// + /// + /// The constructor used to bind and accept on its own, so a caller could not subscribe before + /// clients arrived. Nothing here asserts about sockets: the point is only that constructing + /// is now separable from accepting, which is what lets handlers be bound first. + /// + [TestMethod] + public void TestUConstructingAServerDoesNotStartAccepting() + { + Server server = new Server(AnyLoopbackPort()); + + try + { + Assert.AreEqual( + 0, + server.GetConnectedClientCount(), + "A server that was never told to listen cannot have accepted anyone."); + + // Binding happens here, not in the constructor - and doing it explicitly must work. + server.StartListening(); + + Assert.IsNotNull(server.GetSocket(), "Listening should leave a bound socket behind."); + } + finally + { + server.Stop(); + } + } + + /// + /// Stopping a server that never listened is quiet, and stopping twice is too. + /// + /// + /// CreateMockRippled.Start() races its own Stop(): a mock stopped before startup + /// finishes has its server closed without ever having accepted. That path has to be silent, or + /// the teardown of a fast test becomes a failure of its own. + /// + [TestMethod] + public void TestUStoppingAServerThatNeverListenedIsQuiet() + { + Server server = new Server(AnyLoopbackPort()); + + server.Stop(); + + Assert.ThrowsExactly( + () => server.StartListening(), + "Listening on a socket that Stop() disposed should say so plainly, not carry on half-alive."); + } +} diff --git a/Tests/Xrpl.Tests/CreateMockRippled.cs b/Tests/Xrpl.Tests/CreateMockRippled.cs index 3b7653cb..d6b4c3a0 100644 --- a/Tests/Xrpl.Tests/CreateMockRippled.cs +++ b/Tests/Xrpl.Tests/CreateMockRippled.cs @@ -277,17 +277,10 @@ public void Start() Server server = new Server(new IPEndPoint(IPAddress.Parse("127.0.0.1"), this._port)); - lock (_serverLock) - { - if (_stopped) - { - // Stop() already ran - do not leave this listener accepting behind the test's back. - StopServer(server); - return; - } - - _server = server; - } + // Handlers first, listening afterwards. The server used to start accepting from its + // own constructor, which left a window where a client could connect - and disconnect, + // or send a request - before anything was subscribed. That window is what crashed the + // test host, and it is closed here rather than only survived. // Bind the event for when a client connected server.OnClientConnected += (object sender, OnClientConnectedHandler e) => @@ -392,6 +385,23 @@ public void Start() //e.GetClient().GetServer().ClientDisconnect(e.GetClient()); string clientGuid = e.GetClient().GetGuid(); }; + + lock (_serverLock) + { + if (_stopped) + { + // Stop() already ran - do not leave this listener accepting behind the test's back. + StopServer(server); + return; + } + + _server = server; + + // Inside the lock, so Stop() cannot slip between publishing the server and it + // beginning to accept: whichever takes the lock first wins outright, and a Stop() + // that follows closes a socket that is genuinely listening. + server.StartListening(); + } } } } \ No newline at end of file diff --git a/Tests/Xrpl.Tests/MockRippled/Server.cs b/Tests/Xrpl.Tests/MockRippled/Server.cs index 9af307f5..fd811b11 100644 --- a/Tests/Xrpl.Tests/MockRippled/Server.cs +++ b/Tests/Xrpl.Tests/MockRippled/Server.cs @@ -142,7 +142,16 @@ public partial class Server private IPEndPoint _endPoint; /// The connected clients to the server - private List _clients = new List(); + /// + /// Mutated from socket callbacks, which run on thread-pool threads, and read from the + /// test thread - so every touch goes through . Without it a + /// client connecting while another disconnects can corrupt the list, and enumerating it + /// during either throws on a thread where nothing + /// is left to catch it. + /// + private readonly List _clients = new List(); + + private readonly object _clientsLock = new object(); #endregion @@ -161,9 +170,6 @@ public Server(IPEndPoint EndPoint) //Console.WriteLine("Copyright © 2017 - MazyModz. Created by Dennis Andersson. All rights reserved.\n\n"); //Console.WriteLine("WebSocket Server Started\nListening on {0}:{1}\n", GetEndPoint().Address.ToString(), GetEndPoint().Port); - - // Start the server - start(); } #endregion @@ -189,8 +195,11 @@ public IPEndPoint GetEndPoint() /// The connected client at the index, returns null if the index is out of bounds public MockClient GetConnectedClient(int Index) { - if (Index < 0 || Index >= _clients.Count) return null; - return _clients[Index]; + lock (_clientsLock) + { + if (Index < 0 || Index >= _clients.Count) return null; + return _clients[Index]; + } } /// Gets a connected client with the given guid @@ -198,9 +207,12 @@ public MockClient GetConnectedClient(int Index) /// The client with the given id, return null if no client with the guid could be found public MockClient GetConnectedClient(string Guid) { - foreach (MockClient client in _clients) + lock (_clientsLock) { - if (client.GetGuid() == Guid) return client; + foreach (MockClient client in _clients) + { + if (client.GetGuid() == Guid) return client; + } } return null; } @@ -210,9 +222,12 @@ public MockClient GetConnectedClient(string Guid) /// The connected client with the given socket, returns null if no client with the socket was found public MockClient GetConnectedClient(Socket Socket) { - foreach (MockClient client in _clients) + lock (_clientsLock) { - if (client.GetSocket() == Socket) return client; + foreach (MockClient client in _clients) + { + if (client.GetSocket() == Socket) return client; + } } return null; } @@ -221,7 +236,10 @@ public MockClient GetConnectedClient(Socket Socket) /// The number of connected clients public int GetConnectedClientCount() { - return _clients.Count; + lock (_clientsLock) + { + return _clients.Count; + } } #endregion @@ -229,9 +247,16 @@ public int GetConnectedClientCount() #region Methods /// - /// Starts the listen server when a server object is created + /// Binds the listen socket and starts accepting connections. /// - private void start() + /// + /// Separate from the constructor on purpose. While this ran from there, the socket was + /// accepting before the caller had bound a single handler, so a client that connected in + /// that window raised its events into nothing: the connect was invisible, and a request + /// arriving before OnMessageReceived was bound went unanswered, which a test sees as a + /// timeout rather than as a race. Bind first, then call this. + /// + public void StartListening() { // Bind the socket and start listending GetSocket().Bind(GetEndPoint()); @@ -278,11 +303,15 @@ private void connectionCallback(IAsyncResult AsyncResult) // it to the list of connected clients MockClient client = new MockClient(this, clientSocket); clientSocket = null; - _clients.Add(client); + lock (_clientsLock) + { + _clients.Add(client); + } - // 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)); + // No subscriber is a legitimate state for an event, not a server fault. These + // fire from socket callbacks on thread-pool threads, where a throw is not a + // failed assertion but a dead process - see ClientDisconnect below. + OnClientConnected?.Invoke(this, new OnClientConnectedHandler(client)); } catch (ObjectDisposedException) { @@ -328,8 +357,7 @@ private void connectionCallback(IAsyncResult AsyncResult) /// The message that the client sent public void ReceiveMessage(MockClient Client, string Message) { - if (OnMessageReceived == null) throw new Exception("Server error: event OnMessageReceived is not bound!"); - OnMessageReceived(this, new OnMessageReceivedHandler(Client, Message)); + OnMessageReceived?.Invoke(this, new OnMessageReceivedHandler(Client, Message)); } /// Called when a client disconnectes, calls event OnClientDisconnected @@ -337,11 +365,17 @@ public void ReceiveMessage(MockClient Client, string Message) public void ClientDisconnect(MockClient Client) { // Remove the client from the connected clients list - _clients.Remove(Client); + lock (_clientsLock) + { + _clients.Remove(Client); + } - // Call the OnClientDisconnected event - if (OnClientDisconnected == null) throw new Exception("Server error: OnClientDisconnected is not bound!"); - OnClientDisconnected(this, new OnClientDisconnectedHandler(Client)); + // This used to throw when nothing was subscribed, and it is the reason the whole test + // host died: MockClient.messageCallback calls this from inside its own catch block, on + // a thread-pool thread, so the exception escaped a catch and had nowhere left to go. + // A run aborted that way reports one failed job while an unknown number of tests never + // ran at all - a failure that hides its own size. + OnClientDisconnected?.Invoke(this, new OnClientDisconnectedHandler(Client)); } #endregion @@ -360,8 +394,7 @@ public void SendMessage(MockClient Client, string Data) Client.GetSocket().Send(frameMessage); // Call the on send message callback event - if (OnSendMessage == null) throw new Exception("Server error: event OnSendMessage is not bound!"); - OnSendMessage(this, new OnSendMessageHandler(Client, Data)); + OnSendMessage?.Invoke(this, new OnSendMessageHandler(Client, Data)); } /// Called after a message was sent From b01ea81fb1638f79f2617584c61184be1cda15e7 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Wed, 26 Aug 2026 10:05:38 -0300 Subject: [PATCH 2/3] test(mock): assert the socket is bound, not merely present Review catch. Asserting GetSocket() is non-null passes whether or not StartListening() does anything, which makes it no test of the split it was written to pin. IsBound is the actual invariant: false after construction, true after listening. --- Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs index 013cd95f..81a2b800 100644 --- a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs @@ -129,10 +129,16 @@ public void TestUConstructingAServerDoesNotStartAccepting() server.GetConnectedClientCount(), "A server that was never told to listen cannot have accepted anyone."); + Assert.IsFalse( + server.GetSocket().IsBound, + "The constructor must not bind - that is the whole point of the split."); + // Binding happens here, not in the constructor - and doing it explicitly must work. server.StartListening(); - Assert.IsNotNull(server.GetSocket(), "Listening should leave a bound socket behind."); + Assert.IsTrue( + server.GetSocket().IsBound, + "StartListening must actually bind; asserting the socket is merely non-null would pass even if it did nothing."); } finally { From b95fd7a8ae0ef31e918988211acadffffc56df78 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Wed, 26 Aug 2026 10:51:35 -0300 Subject: [PATCH 3/3] test(mock): make the concurrency test actually pin the lock it names Review findings. TestUTheClientListToleratesConcurrentUse pinned nothing. It churned ClientDisconnect(null) against readers, but the list then stays empty, and List.Remove of an absent element returns without touching the version counter an enumerator checks - so no mutation ever raced an enumeration. The test passed with _clientsLock removed outright; verified by replacing it with a fresh object per access, which disables mutual exclusion entirely. It now drives real clients over loopback sockets: two threads add and remove while two more walk the list end to end. Same mutation now fails it, which is the whole point of writing it down. TrackClient() is extracted from the accept callback so the add side is reachable without a socket handshake. It is the same code, under the same lock. The handshake read in connectionCallback is now bounded at five seconds. BeginAccept can complete synchronously when a connection is already pending, in which case that blocking read runs on the thread that called StartListening - which holds _serverLock - so a client that connects and then says nothing would hold up Stop() indefinitely. Narrow, but this change exists to remove a hang. And TestUStoppingAServerThatNeverListenedIsQuiet said "and stopping twice is too" while stopping once. It stops twice now. --- .../Client/TestUMockRippledServer.cs | 70 ++++++++++++++++--- Tests/Xrpl.Tests/MockRippled/Server.cs | 30 ++++++-- 2 files changed, 84 insertions(+), 16 deletions(-) diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs index 81a2b800..5d7fba23 100644 --- a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Net; +using System.Net.Sockets; using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -62,41 +63,62 @@ public void TestUAnUnsubscribedEventIsNotAnError() } /// - /// The client list survives being touched from several threads at once. + /// The client list survives being added to, removed from and read at once. /// /// + /// /// _clients is added to from the accept callback and removed from on disconnect - both - /// thread-pool threads - while the test thread reads it through GetConnectedClient and - /// GetConnectedClientCount. Unsynchronised, an add during an enumeration throws - /// on a thread with no catch above it, which is the - /// same fatal shape as the bug above by a different route. + /// thread-pool threads - while the test thread reads it through GetConnectedClient. + /// Unsynchronised, a mutation during an enumeration throws + /// on a thread with no catch above it: the same fatal + /// shape as the crash above, by a different route. + /// + /// + /// The clients here are real, and that is the point. An earlier version of this test spun + /// ClientDisconnect(null) against readers, which pins nothing: the list stays empty, and + /// List<T>.Remove of an absent element returns without touching the version counter + /// the enumerator checks. That test passed with _clientsLock removed outright. This one + /// does not. + /// /// [TestMethod] public async Task TestUTheClientListToleratesConcurrentUse() { Server server = new Server(AnyLoopbackPort()); + List sockets = new List(); try { + MockClient[] clients = new MockClient[8]; + for (int i = 0; i < clients.Length; i++) + { + clients[i] = new MockClient(server, ConnectedSocket(sockets)); + } + List workers = new List(); - for (int i = 0; i < 4; i++) + // Two threads churn the list while two more walk it end to end. + for (int worker = 0; worker < 2; worker++) { + int offset = worker * 4; + workers.Add(Task.Run(() => { - for (int n = 0; n < 2_000; n++) + for (int n = 0; n < 20_000; n++) { - server.ClientDisconnect(null); + MockClient client = clients[offset + (n % 4)]; + server.TrackClient(client); + server.ClientDisconnect(client); } })); workers.Add(Task.Run(() => { - for (int n = 0; n < 2_000; n++) + for (int n = 0; n < 20_000; n++) { - server.GetConnectedClientCount(); - server.GetConnectedClient(0); + // Enumerates to the end, because no client carries this guid. server.GetConnectedClient("no-such-guid"); + server.GetConnectedClientCount(); } })); } @@ -105,10 +127,35 @@ public async Task TestUTheClientListToleratesConcurrentUse() } finally { + foreach (Socket socket in sockets) + { + try { socket.Close(); } catch { } + } + server.Stop(); } } + /// + /// A connected loopback socket, so a can be built without a handshake. + /// + private static Socket ConnectedSocket(List toClose) + { + Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + listener.Bind(new IPEndPoint(IPAddress.Loopback, 0)); + listener.Listen(1); + + Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + client.Connect((IPEndPoint)listener.LocalEndPoint); + + Socket accepted = listener.Accept(); + listener.Close(); + + toClose.Add(client); + toClose.Add(accepted); + return accepted; + } + /// /// A server that has not been told to listen is not listening. /// @@ -159,6 +206,7 @@ public void TestUStoppingAServerThatNeverListenedIsQuiet() { Server server = new Server(AnyLoopbackPort()); + server.Stop(); server.Stop(); Assert.ThrowsExactly( diff --git a/Tests/Xrpl.Tests/MockRippled/Server.cs b/Tests/Xrpl.Tests/MockRippled/Server.cs index fd811b11..3a0a5b17 100644 --- a/Tests/Xrpl.Tests/MockRippled/Server.cs +++ b/Tests/Xrpl.Tests/MockRippled/Server.cs @@ -288,7 +288,14 @@ private void connectionCallback(IAsyncResult AsyncResult) // Gets the client thats trying to connect to the server clientSocket = GetSocket().EndAccept(AsyncResult); - // Read the handshake updgrade request + // Read the handshake updgrade request. + // Bounded on purpose: BeginAccept can complete synchronously when a connection is + // already pending, in which case this callback - and this blocking read - runs on + // the thread that called StartListening, which holds _serverLock. A client that + // connects and then says nothing would hold up Stop() for as long as it stayed + // quiet. Five seconds is far longer than a real handshake and far shorter than + // forever. + clientSocket.ReceiveTimeout = 5000; byte[] handshakeBuffer = new byte[1024]; int handshakeReceived = clientSocket.Receive(handshakeBuffer); @@ -303,10 +310,7 @@ private void connectionCallback(IAsyncResult AsyncResult) // it to the list of connected clients MockClient client = new MockClient(this, clientSocket); clientSocket = null; - lock (_clientsLock) - { - _clients.Add(client); - } + TrackClient(client); // No subscriber is a legitimate state for an event, not a server fault. These // fire from socket callbacks on thread-pool threads, where a throw is not a @@ -360,6 +364,22 @@ public void ReceiveMessage(MockClient Client, string Message) OnMessageReceived?.Invoke(this, new OnMessageReceivedHandler(Client, Message)); } + /// Records a connected client. Paired with . + /// + /// Extracted from the accept callback so a test can drive the add side of the list without a + /// socket handshake. Without it the only reachable mutation is removing a client that is not + /// there, and List<T>.Remove leaves its version counter alone in that case - so a + /// concurrency test built on it exercises no mutual exclusion at all and passes with the lock + /// removed entirely. + /// + internal void TrackClient(MockClient Client) + { + lock (_clientsLock) + { + _clients.Add(Client); + } + } + /// Called when a client disconnectes, calls event OnClientDisconnected /// The client that disconnected public void ClientDisconnect(MockClient Client)