diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs new file mode 100644 index 00000000..5d7fba23 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs @@ -0,0 +1,216 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Sockets; +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 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. + /// 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(); + + // 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 < 20_000; n++) + { + MockClient client = clients[offset + (n % 4)]; + server.TrackClient(client); + server.ClientDisconnect(client); + } + })); + + workers.Add(Task.Run(() => + { + for (int n = 0; n < 20_000; n++) + { + // Enumerates to the end, because no client carries this guid. + server.GetConnectedClient("no-such-guid"); + server.GetConnectedClientCount(); + } + })); + } + + await Task.WhenAll(workers); + } + 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. + /// + /// + /// 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."); + + 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.IsTrue( + server.GetSocket().IsBound, + "StartListening must actually bind; asserting the socket is merely non-null would pass even if it did nothing."); + } + 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(); + 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..3a0a5b17 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()); @@ -263,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); @@ -278,11 +310,12 @@ private void connectionCallback(IAsyncResult AsyncResult) // it to the list of connected clients MockClient client = new MockClient(this, clientSocket); clientSocket = null; - _clients.Add(client); + TrackClient(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 +361,23 @@ 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)); + } + + /// 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 @@ -337,11 +385,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 +414,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