From 0cfeb6bb943adff9a56d863dfffcf45224769af7 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Fri, 10 May 2024 10:37:01 +0100 Subject: [PATCH] Tweak AsyncSocketListener in tests (#1382) * Tweak AsyncSocketListener in tests Some CI runs have been crashing lately from within AsyncSocketListener. This moves a bit of disconnection & exception handling around. It probably won't fix the underlying tests which would have otherwise failed, but it might stop the process from outright crashing. Also reference the same code for the integration tests. * Simplify the diff * Stabilise a couple of tests --- .../Common/AsyncSocketListener.cs | 393 ------------------ .../OldIntegrationTests/SshCommandTest.cs | 16 +- .../Renci.SshNet.IntegrationTests.csproj | 4 + .../Renci.SshNet.IntegrationTests/SshTests.cs | 1 + .../Common/AsyncSocketListener.cs | 17 +- 5 files changed, 24 insertions(+), 407 deletions(-) delete mode 100644 test/Renci.SshNet.IntegrationTests/Common/AsyncSocketListener.cs diff --git a/test/Renci.SshNet.IntegrationTests/Common/AsyncSocketListener.cs b/test/Renci.SshNet.IntegrationTests/Common/AsyncSocketListener.cs deleted file mode 100644 index 753385582..000000000 --- a/test/Renci.SshNet.IntegrationTests/Common/AsyncSocketListener.cs +++ /dev/null @@ -1,393 +0,0 @@ -using System.Net; -using System.Net.Sockets; - -namespace Renci.SshNet.IntegrationTests.Common -{ - public class AsyncSocketListener : IDisposable - { - private readonly IPEndPoint _endPoint; - private readonly ManualResetEvent _acceptCallbackDone; - private readonly List _connectedClients; - private readonly object _syncLock; - private Socket _listener; - private Thread _receiveThread; - private bool _started; - private string _stackTrace; - - public delegate void BytesReceivedHandler(byte[] bytesReceived, Socket socket); - public delegate void ConnectedHandler(Socket socket); - - public event BytesReceivedHandler BytesReceived; - public event ConnectedHandler Connected; - public event ConnectedHandler Disconnected; - - public AsyncSocketListener(IPEndPoint endPoint) - { - _endPoint = endPoint; - _acceptCallbackDone = new ManualResetEvent(false); - _connectedClients = new List(); - _syncLock = new object(); - ShutdownRemoteCommunicationSocket = true; - } - - /// - /// Gets a value indicating whether the is invoked on the - /// that is used to handle the communication with the remote host, when the remote host has closed the connection. - /// - /// - /// to invoke on the that is used - /// to handle the communication with the remote host, when the remote host has closed the connection; otherwise, - /// . The default is . - /// - public bool ShutdownRemoteCommunicationSocket { get; set; } - - public void Start() - { - _listener = new Socket(_endPoint.AddressFamily, SocketType.Stream, ProtocolType.Tcp); - _listener.Bind(_endPoint); - _listener.Listen(1); - - _started = true; - - _receiveThread = new Thread(StartListener); - _receiveThread.Start(_listener); - - _stackTrace = Environment.StackTrace; - } - - public void Stop() - { - _started = false; - - lock (_syncLock) - { - foreach (var connectedClient in _connectedClients) - { - try - { - connectedClient.Shutdown(SocketShutdown.Send); - } - catch (Exception ex) - { - Console.Error.WriteLine("[{0}] Failure shutting down socket: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - - DrainSocket(connectedClient); - connectedClient.Dispose(); - } - - _connectedClients.Clear(); - } - - _listener?.Dispose(); - - if (_receiveThread != null) - { - _receiveThread.Join(); - _receiveThread = null; - } - } - - public void Dispose() - { - Stop(); - GC.SuppressFinalize(this); - } - - private void StartListener(object state) - { - try - { - var listener = (Socket) state; - while (_started) - { - _ = _acceptCallbackDone.Reset(); - _ = listener.BeginAccept(AcceptCallback, listener); - _ = _acceptCallbackDone.WaitOne(); - } - } - catch (Exception ex) - { - // On .NET framework when Thread throws an exception then unit tests - // were executed without any problem. - // On new .NET exceptions from Thread breaks unit tests session. - Console.Error.WriteLine("[{0}] Failure in StartListener: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - } - - private void AcceptCallback(IAsyncResult ar) - { - // Signal the main thread to continue - _ = _acceptCallbackDone.Set(); - - // Get the socket that listens for inbound connections - var listener = (Socket) ar.AsyncState; - - // Get the socket that handles the client request - Socket handler; - - try - { - handler = listener.EndAccept(ar); - } - catch (SocketException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.EndAccept(...) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure accepting new connection: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - return; - } - catch (ObjectDisposedException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.EndAccept(IAsyncResult) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure accepting new connection: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - return; - } - - // Signal new connection - SignalConnected(handler); - - lock (_syncLock) - { - // Register client socket - _connectedClients.Add(handler); - } - - var state = new SocketStateObject(handler); - - try - { - _ = handler.BeginReceive(state.Buffer, 0, state.Buffer.Length, 0, ReadCallback, state); - } - catch (SocketException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.BeginReceive(...) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure receiving new data: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - } - catch (ObjectDisposedException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.BeginReceive(...) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure receiving new data: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - } - } - - private void ReadCallback(IAsyncResult ar) - { - // Retrieve the state object and the handler socket - // from the asynchronous state object - var state = (SocketStateObject) ar.AsyncState; - var handler = state.Socket; - - int bytesRead; - try - { - // Read data from the client socket. - bytesRead = handler.EndReceive(ar, out var errorCode); - if (errorCode != SocketError.Success) - { - bytesRead = 0; - } - } - catch (SocketException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.EndReceive(...) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure receiving new data: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - return; - } - catch (ObjectDisposedException ex) - { - // The listener is stopped through a Dispose() call, which in turn causes - // Socket.EndReceive(...) to throw a SocketException or - // ObjectDisposedException - // - // Since we consider such an exception normal when the listener is being - // stopped, we only write a message to stderr if the listener is considered - // to be up and running - if (_started) - { - Console.Error.WriteLine("[{0}] Failure receiving new data: {1}", - typeof(AsyncSocketListener).FullName, - ex); - } - return; - } - - void ConnectionDisconnected() - { - SignalDisconnected(handler); - - if (ShutdownRemoteCommunicationSocket) - { - lock (_syncLock) - { - if (!_started) - { - return; - } - - try - { - handler.Shutdown(SocketShutdown.Send); - handler.Close(); - } - catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) - { - // On .NET 7 we got Socker Exception with ConnectionReset from Shutdown method - // when the socket is disposed - } - catch (SocketException ex) - { - throw new Exception("Exception in ReadCallback: " + ex.SocketErrorCode + " " + _stackTrace, ex); - } - catch (Exception ex) - { - throw new Exception("Exception in ReadCallback: " + _stackTrace, ex); - } - - _ = _connectedClients.Remove(handler); - } - } - } - - if (bytesRead > 0) - { - var bytesReceived = new byte[bytesRead]; - Array.Copy(state.Buffer, bytesReceived, bytesRead); - SignalBytesReceived(bytesReceived, handler); - - try - { - _ = handler.BeginReceive(state.Buffer, 0, state.Buffer.Length, 0, ReadCallback, state); - } - catch (ObjectDisposedException) - { - // TODO On .NET 7, sometimes we get ObjectDisposedException when _started but only on appveyor, locally it works - ConnectionDisconnected(); - } - catch (SocketException ex) - { - if (!_started) - { - throw new Exception("BeginReceive while stopping!", ex); - } - - throw new Exception("BeginReceive while started!: " + ex.SocketErrorCode + " " + _stackTrace, ex); - } - - } - else - { - ConnectionDisconnected(); - } - } - - private void SignalBytesReceived(byte[] bytesReceived, Socket client) - { - BytesReceived?.Invoke(bytesReceived, client); - } - - private void SignalConnected(Socket client) - { - Connected?.Invoke(client); - } - - private void SignalDisconnected(Socket client) - { - Disconnected?.Invoke(client); - } - - private static void DrainSocket(Socket socket) - { - var buffer = new byte[128]; - - try - { - while (true && socket.Connected) - { - var bytesRead = socket.Receive(buffer); - if (bytesRead == 0) - { - break; - } - } - } - catch (SocketException ex) - { - Console.Error.WriteLine("[{0}] Failure draining socket ({1}): {2}", - typeof(AsyncSocketListener).FullName, - ex.SocketErrorCode.ToString("G"), - ex); - } - } - - private class SocketStateObject - { - public Socket Socket { get; private set; } - - public readonly byte[] Buffer = new byte[1024]; - - public SocketStateObject(Socket handler) - { - Socket = handler; - } - } - } -} diff --git a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SshCommandTest.cs b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SshCommandTest.cs index 4273339f4..f9ef378cd 100644 --- a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SshCommandTest.cs +++ b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SshCommandTest.cs @@ -326,19 +326,17 @@ public void Test_Execute_Command_Asynchronously_With_Callback() { client.Connect(); - var callbackCalled = false; + using var callbackCalled = new ManualResetEventSlim(); - var cmd = client.CreateCommand("sleep 5s; echo 'test'"); + using var cmd = client.CreateCommand("sleep 5s; echo 'test'"); var asyncResult = cmd.BeginExecute(new AsyncCallback((s) => { - callbackCalled = true; + callbackCalled.Set(); }), null); cmd.EndExecute(asyncResult); - Thread.Sleep(100); - - Assert.IsTrue(callbackCalled); + Assert.IsTrue(callbackCalled.Wait(TimeSpan.FromSeconds(1))); client.Disconnect(); } @@ -353,16 +351,18 @@ public void Test_Execute_Command_Asynchronously_With_Callback_On_Different_Threa var currentThreadId = Thread.CurrentThread.ManagedThreadId; int callbackThreadId = 0; + using var callbackCalled = new ManualResetEventSlim(); - var cmd = client.CreateCommand("sleep 5s; echo 'test'"); + using var cmd = client.CreateCommand("sleep 5s; echo 'test'"); var asyncResult = cmd.BeginExecute(new AsyncCallback((s) => { callbackThreadId = Thread.CurrentThread.ManagedThreadId; + callbackCalled.Set(); }), null); cmd.EndExecute(asyncResult); - Thread.Sleep(100); + Assert.IsTrue(callbackCalled.Wait(TimeSpan.FromSeconds(1))); Assert.AreNotEqual(currentThreadId, callbackThreadId); diff --git a/test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj b/test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj index f97567b9a..8cb42da93 100644 --- a/test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj +++ b/test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj @@ -8,6 +8,10 @@ true + + + + diff --git a/test/Renci.SshNet.IntegrationTests/SshTests.cs b/test/Renci.SshNet.IntegrationTests/SshTests.cs index fe05e3482..3cf4a9a10 100644 --- a/test/Renci.SshNet.IntegrationTests/SshTests.cs +++ b/test/Renci.SshNet.IntegrationTests/SshTests.cs @@ -4,6 +4,7 @@ using Renci.SshNet.Common; using Renci.SshNet.IntegrationTests.Common; +using Renci.SshNet.Tests.Common; namespace Renci.SshNet.IntegrationTests { diff --git a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs index 59317f44f..733d7e51a 100644 --- a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs +++ b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs @@ -1,8 +1,10 @@ -using System; +#pragma warning disable IDE0005 // Using directive is unnecessary; IntegrationTests use implicit usings +using System; using System.Collections.Generic; using System.Net; using System.Net.Sockets; using System.Threading; +#pragma warning restore IDE0005 namespace Renci.SshNet.Tests.Common { @@ -273,7 +275,7 @@ private void ReadCallback(IAsyncResult ar) return; } - void ConnectionDisconnected() + void ConnectionDisconnected(bool doShutdownIfApplicable) { SignalDisconnected(handler); @@ -288,8 +290,11 @@ void ConnectionDisconnected() try { - handler.Shutdown(SocketShutdown.Send); - handler.Close(); + if (doShutdownIfApplicable) + { + handler.Shutdown(SocketShutdown.Send); + handler.Close(); + } } catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) { @@ -323,7 +328,7 @@ void ConnectionDisconnected() catch (ObjectDisposedException) { // TODO On .NET 7, sometimes we get ObjectDisposedException when _started but only on appveyor, locally it works - ConnectionDisconnected(); + ConnectionDisconnected(doShutdownIfApplicable: false); } catch (SocketException ex) { @@ -338,7 +343,7 @@ void ConnectionDisconnected() } else { - ConnectionDisconnected(); + ConnectionDisconnected(doShutdownIfApplicable: true); } }