From b3ccc0fd5cd73a5b0774510da3a2cc9aca4a1cf9 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 25 Apr 2024 08:12:42 +0200 Subject: [PATCH 1/3] 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. --- .../Common/AsyncSocketListener.cs | 393 ------------------ .../Renci.SshNet.IntegrationTests.csproj | 4 + .../Renci.SshNet.IntegrationTests/SshTests.cs | 1 + .../Common/AsyncSocketListener.cs | 77 ++-- 4 files changed, 31 insertions(+), 444 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/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..7f8d23e39 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 { @@ -15,7 +17,6 @@ public class AsyncSocketListener : IDisposable 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); @@ -54,8 +55,6 @@ public void Start() _receiveThread = new Thread(StartListener); _receiveThread.Start(_listener); - - _stackTrace = Environment.StackTrace; } public void Stop() @@ -273,43 +272,6 @@ private void ReadCallback(IAsyncResult ar) 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]; @@ -322,23 +284,36 @@ void ConnectionDisconnected() } catch (ObjectDisposedException) { - // TODO On .NET 7, sometimes we get ObjectDisposedException when _started but only on appveyor, locally it works - ConnectionDisconnected(); + SignalDisconnected(handler); } - catch (SocketException ex) + + return; + } + + SignalDisconnected(handler); + + if (ShutdownRemoteCommunicationSocket) + { + lock (_syncLock) { if (!_started) { - throw new Exception("BeginReceive while stopping!", ex); + return; } - throw new Exception("BeginReceive while started!: " + ex.SocketErrorCode + " " + _stackTrace, ex); - } + try + { + handler.Shutdown(SocketShutdown.Send); + handler.Close(); + } + catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) + { + // On .NET 7 we got Socket Exception with ConnectionReset from Shutdown method + // when the socket is disposed + } - } - else - { - ConnectionDisconnected(); + _ = _connectedClients.Remove(handler); + } } } From 0830d14a1dc18b6f3e7937fd1eb90d8f69ede460 Mon Sep 17 00:00:00 2001 From: Robert Hague Date: Wed, 8 May 2024 14:32:13 +0200 Subject: [PATCH 2/3] Simplify the diff --- .../Common/AsyncSocketListener.cs | 76 +++++++++++++------ 1 file changed, 53 insertions(+), 23 deletions(-) diff --git a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs index 7f8d23e39..733d7e51a 100644 --- a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs +++ b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs @@ -17,6 +17,7 @@ public class AsyncSocketListener : IDisposable 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); @@ -55,6 +56,8 @@ public void Start() _receiveThread = new Thread(StartListener); _receiveThread.Start(_listener); + + _stackTrace = Environment.StackTrace; } public void Stop() @@ -272,6 +275,46 @@ private void ReadCallback(IAsyncResult ar) return; } + void ConnectionDisconnected(bool doShutdownIfApplicable) + { + SignalDisconnected(handler); + + if (ShutdownRemoteCommunicationSocket) + { + lock (_syncLock) + { + if (!_started) + { + return; + } + + try + { + if (doShutdownIfApplicable) + { + 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]; @@ -284,36 +327,23 @@ private void ReadCallback(IAsyncResult ar) } catch (ObjectDisposedException) { - SignalDisconnected(handler); + // TODO On .NET 7, sometimes we get ObjectDisposedException when _started but only on appveyor, locally it works + ConnectionDisconnected(doShutdownIfApplicable: false); } - - return; - } - - SignalDisconnected(handler); - - if (ShutdownRemoteCommunicationSocket) - { - lock (_syncLock) + catch (SocketException ex) { if (!_started) { - return; - } - - try - { - handler.Shutdown(SocketShutdown.Send); - handler.Close(); - } - catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) - { - // On .NET 7 we got Socket Exception with ConnectionReset from Shutdown method - // when the socket is disposed + throw new Exception("BeginReceive while stopping!", ex); } - _ = _connectedClients.Remove(handler); + throw new Exception("BeginReceive while started!: " + ex.SocketErrorCode + " " + _stackTrace, ex); } + + } + else + { + ConnectionDisconnected(doShutdownIfApplicable: true); } } From 5ed4b2e537a40529404602980682ca178e93000b Mon Sep 17 00:00:00 2001 From: Robert Hague Date: Thu, 9 May 2024 09:32:53 +0200 Subject: [PATCH 3/3] Stabilise a couple of tests --- .../OldIntegrationTests/SshCommandTest.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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);