From c722542b044b1f19a58543f38b4b1bddd170d2b2 Mon Sep 17 00:00:00 2001 From: Alex Johnson Date: Fri, 2 Sep 2022 14:50:15 -0400 Subject: [PATCH] Adds lock to ssh and tcp classes to prevent possible threading issues. --- .../Pepperdash Core/Comm/GenericSshClient.cs | 262 +++++++++--------- .../Comm/GenericTcpIpClient.cs | 139 +++++----- .../Pepperdash Core/Logging/Debug.cs | 4 - 3 files changed, 204 insertions(+), 201 deletions(-) diff --git a/Pepperdash Core/Pepperdash Core/Comm/GenericSshClient.cs b/Pepperdash Core/Pepperdash Core/Comm/GenericSshClient.cs index 5f4c5c8..54ff6c9 100644 --- a/Pepperdash Core/Pepperdash Core/Comm/GenericSshClient.cs +++ b/Pepperdash Core/Pepperdash Core/Comm/GenericSshClient.cs @@ -14,6 +14,10 @@ namespace PepperDash.Core public class GenericSshClient : Device, ISocketStatusWithStreamDebugging, IAutoReconnect { private const string SPlusKey = "Uninitialized SshClient"; + + /// + /// Enables debugging to console + /// public CommunicationStreamDebugging StreamDebugging { get; private set; } /// @@ -31,11 +35,6 @@ namespace PepperDash.Core /// public event EventHandler ConnectionChange; - /// - /// - /// - //public event GenericSocketStatusChangeEventDelegate SocketStatusChange; - /// /// Address of server /// @@ -62,12 +61,9 @@ namespace PepperDash.Core public bool IsConnected { // returns false if no client or not connected - get { return ClientStatus == SocketStatus.SOCKET_STATUS_CONNECTED; } + get { return Client != null && ClientStatus == SocketStatus.SOCKET_STATUS_CONNECTED; } } - private bool IsConnecting = false; - private bool DisconnectLogged = false; - /// /// S+ helper for IsConnected /// @@ -132,10 +128,10 @@ namespace PepperDash.Core CTimer ReconnectTimer; - //string PreviousHostname; - //int PreviousPort; - //string PreviousUsername; - //string PreviousPassword; + //Lock object to prevent simulatneous connect/disconnect operations + private readonly object connectLock = new object(); + + private bool DisconnectLogged = false; /// /// Typical constructor. @@ -154,7 +150,10 @@ namespace PepperDash.Core ReconnectTimer = new CTimer(o => { - Connect(); + if (ConnectEnabled) + { + Connect(); + } }, Timeout.Infinite); } @@ -170,7 +169,10 @@ namespace PepperDash.Core ReconnectTimer = new CTimer(o => { - Connect(); + if (ConnectEnabled) + { + Connect(); + } }, Timeout.Infinite); } @@ -202,110 +204,111 @@ namespace PepperDash.Core /// public void Connect() { - if (IsConnecting) - { - Debug.Console(0, this, Debug.ErrorLogLevel.Warning, "Connection attempt in progress. Exiting Connect()"); - return; - } - - IsConnecting = true; - ConnectEnabled = true; - Debug.Console(1, this, "attempting connect"); - - // Cancel reconnect if running. - ReconnectTimer.Stop(); - - // Don't try to connect if already - if (IsConnected) - return; - // Don't go unless everything is here if (string.IsNullOrEmpty(Hostname) || Port < 1 || Port > 65535 || Username == null || Password == null) { - Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Connect failed. Check hostname, port, username and password are set or not null"); + Debug.Console(0, this, Debug.ErrorLogLevel.Error, "Connect failed. Check hostname, port, username and password are set or not null"); return; } - // Cleanup the old client if it already exists - if (Client != null) + ConnectEnabled = true; + lock (connectLock) { - Debug.Console(1, this, "Cleaning up disconnected client"); - Client.ErrorOccurred -= Client_ErrorOccurred; - KillClient(SocketStatus.SOCKET_STATUS_BROKEN_LOCALLY); - } - - // This handles both password and keyboard-interactive (like on OS-X, 'nixes) - KeyboardInteractiveAuthenticationMethod kauth = new KeyboardInteractiveAuthenticationMethod(Username); - kauth.AuthenticationPrompt += new EventHandler(kauth_AuthenticationPrompt); - PasswordAuthenticationMethod pauth = new PasswordAuthenticationMethod(Username, Password); - - Debug.Console(1, this, "Creating new SshClient"); - ConnectionInfo connectionInfo = new ConnectionInfo(Hostname, Port, Username, pauth, kauth); - Client = new SshClient(connectionInfo); - - Client.ErrorOccurred -= Client_ErrorOccurred; - Client.ErrorOccurred += Client_ErrorOccurred; - - //Attempt to connect - ClientStatus = SocketStatus.SOCKET_STATUS_WAITING; - try - { - Client.Connect(); - TheStream = Client.CreateShellStream("PDTShell", 100, 80, 100, 200, 65534); - TheStream.DataReceived += Stream_DataReceived; - //TheStream.ErrorOccurred += TheStream_ErrorOccurred; - Debug.Console(1, this, Debug.ErrorLogLevel.Notice, "Connected"); - ClientStatus = SocketStatus.SOCKET_STATUS_CONNECTED; - IsConnecting = false; - DisconnectLogged = false; - return; // Success will not pass here - } - catch (SshConnectionException e) - { - var ie = e.InnerException; // The details are inside!! - var errorLogLevel = DisconnectLogged == true ? Debug.ErrorLogLevel.None : Debug.ErrorLogLevel.Error; - - if (ie is SocketException) - Debug.Console(1, this, errorLogLevel, "'{0}' CONNECTION failure: Cannot reach host, ({1})", Key, ie.Message); - else if (ie is System.Net.Sockets.SocketException) - Debug.Console(1, this, errorLogLevel, "'{0}' Connection failure: Cannot reach host '{1}' on port {2}, ({3})", - Key, Hostname, Port, ie.GetType()); - else if (ie is SshAuthenticationException) + if (IsConnected) { - Debug.Console(1, this, errorLogLevel, "Authentication failure for username '{0}', ({1})", - Username, ie.Message); + Debug.Console(0, this, Debug.ErrorLogLevel.Warning, "Connection already connected. Exiting Connect()"); } else - Debug.Console(1, this, errorLogLevel, "Error on connect:\r({0})", e); + { + Debug.Console(1, this, "Attempting connect"); - DisconnectLogged = true; - ClientStatus = SocketStatus.SOCKET_STATUS_CONNECT_FAILED; - HandleConnectionFailure(); - } - catch (Exception e) - { - Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Unhandled exception on connect:\r({0})", e); - ClientStatus = SocketStatus.SOCKET_STATUS_CONNECT_FAILED; - HandleConnectionFailure(); - } + // Cancel reconnect if running. + ReconnectTimer.Stop(); - ClientStatus = SocketStatus.SOCKET_STATUS_CONNECT_FAILED; - HandleConnectionFailure(); + // Cleanup the old client if it already exists + if (Client != null) + { + Debug.Console(1, this, "Cleaning up disconnected client"); + KillClient(SocketStatus.SOCKET_STATUS_BROKEN_LOCALLY); + } + + // This handles both password and keyboard-interactive (like on OS-X, 'nixes) + KeyboardInteractiveAuthenticationMethod kauth = new KeyboardInteractiveAuthenticationMethod(Username); + kauth.AuthenticationPrompt += new EventHandler(kauth_AuthenticationPrompt); + PasswordAuthenticationMethod pauth = new PasswordAuthenticationMethod(Username, Password); + + Debug.Console(1, this, "Creating new SshClient"); + ConnectionInfo connectionInfo = new ConnectionInfo(Hostname, Port, Username, pauth, kauth); + Client = new SshClient(connectionInfo); + + Client.ErrorOccurred -= Client_ErrorOccurred; + Client.ErrorOccurred += Client_ErrorOccurred; + + //Attempt to connect + ClientStatus = SocketStatus.SOCKET_STATUS_WAITING; + try + { + Client.Connect(); + TheStream = Client.CreateShellStream("PDTShell", 100, 80, 100, 200, 65534); + TheStream.DataReceived += Stream_DataReceived; + Debug.Console(1, this, Debug.ErrorLogLevel.Notice, "Connected"); + ClientStatus = SocketStatus.SOCKET_STATUS_CONNECTED; + DisconnectLogged = false; + } + catch (SshConnectionException e) + { + var ie = e.InnerException; // The details are inside!! + var errorLogLevel = DisconnectLogged == true ? Debug.ErrorLogLevel.None : Debug.ErrorLogLevel.Error; + + if (ie is SocketException) + Debug.Console(1, this, errorLogLevel, "'{0}' CONNECTION failure: Cannot reach host, ({1})", Key, ie.Message); + else if (ie is System.Net.Sockets.SocketException) + Debug.Console(1, this, errorLogLevel, "'{0}' Connection failure: Cannot reach host '{1}' on port {2}, ({3})", + Key, Hostname, Port, ie.GetType()); + else if (ie is SshAuthenticationException) + { + Debug.Console(1, this, errorLogLevel, "Authentication failure for username '{0}', ({1})", + Username, ie.Message); + } + else + Debug.Console(1, this, errorLogLevel, "Error on connect:\r({0})", e); + + DisconnectLogged = true; + KillClient(SocketStatus.SOCKET_STATUS_CONNECT_FAILED); + if (AutoReconnect) + { + Debug.Console(1, this, "Checking autoreconnect: {0}, {1}ms", AutoReconnect, AutoReconnectIntervalMs); + ReconnectTimer.Reset(AutoReconnectIntervalMs); + } + } + catch (Exception e) + { + var errorLogLevel = DisconnectLogged == true ? Debug.ErrorLogLevel.None : Debug.ErrorLogLevel.Error; + Debug.Console(1, this, errorLogLevel, "Unhandled exception on connect:\r({0})", e); + DisconnectLogged = true; + KillClient(SocketStatus.SOCKET_STATUS_CONNECT_FAILED); + if (AutoReconnect) + { + Debug.Console(1, this, "Checking autoreconnect: {0}, {1}ms", AutoReconnect, AutoReconnectIntervalMs); + ReconnectTimer.Reset(AutoReconnectIntervalMs); + } + } + } + } } - - /// /// Disconnect the clients and put away it's resources. /// public void Disconnect() { - ConnectEnabled = false; - // Stop trying reconnects, if we are - ReconnectTimer.Stop(); - - KillClient(SocketStatus.SOCKET_STATUS_BROKEN_LOCALLY); + lock(connectLock) + { + // Stop trying reconnects, if we are + ReconnectTimer.Stop(); + KillClient(SocketStatus.SOCKET_STATUS_BROKEN_LOCALLY); + } } /// @@ -317,31 +320,21 @@ namespace PepperDash.Core if (Client != null) { - IsConnecting = false; - Client.Disconnect(); - Client = null; - ClientStatus = status; - Debug.Console(1, this, "Disconnected"); + try + { + Client.Disconnect(); + Client.Dispose(); + Client = null; + ClientStatus = status; + Debug.Console(1, this, "Disconnected client"); + } + catch (Exception ex) + { + Debug.Console(0, this, "Exception killing client: {0}", ex.Message); + } } } - /// - /// Anything to do with reestablishing connection on failures - /// - void HandleConnectionFailure() - { - KillClient(SocketStatus.SOCKET_STATUS_CONNECT_FAILED); - - Debug.Console(1, this, "Client nulled due to connection failure. AutoReconnect: {0}, ConnectEnabled: {1}", AutoReconnect, ConnectEnabled); - if (AutoReconnect && ConnectEnabled) - { - Debug.Console(1, this, "Checking autoreconnect: {0}, {1}ms", AutoReconnect, AutoReconnectIntervalMs); - ReconnectTimer.Reset(AutoReconnectIntervalMs); - Debug.Console(1, this, "Attempting connection in {0} seconds", - (float) (AutoReconnectIntervalMs/1000)); - } - } - /// /// Kills the stream /// @@ -353,6 +346,7 @@ namespace PepperDash.Core TheStream.Close(); TheStream.Dispose(); TheStream = null; + Debug.Console(1, this, "Disconnected stream"); } } @@ -403,13 +397,23 @@ namespace PepperDash.Core /// void Client_ErrorOccurred(object sender, Crestron.SimplSharp.Ssh.Common.ExceptionEventArgs e) { - if (e.Exception is SshConnectionException || e.Exception is System.Net.Sockets.SocketException) - Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Disconnected by remote"); - else - Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Unhandled SSH client error: {0}", e.Exception); + CrestronInvoke.BeginInvoke(o => + { + if (e.Exception is SshConnectionException || e.Exception is System.Net.Sockets.SocketException) + Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Disconnected by remote"); + else + Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Unhandled SSH client error: {0}", e.Exception); - ClientStatus = SocketStatus.SOCKET_STATUS_BROKEN_REMOTELY; - HandleConnectionFailure(); + lock (connectLock) + { + KillClient(SocketStatus.SOCKET_STATUS_BROKEN_REMOTELY); + } + if (AutoReconnect && ConnectEnabled) + { + Debug.Console(1, this, "Checking autoreconnect: {0}, {1}ms", AutoReconnect, AutoReconnectIntervalMs); + ReconnectTimer.Reset(AutoReconnectIntervalMs); + } + }); } /// @@ -451,8 +455,6 @@ namespace PepperDash.Core Debug.Console(0, "Stack Trace: {0}", ex.StackTrace); Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Stream write failed. Disconnected, closing"); - ClientStatus = SocketStatus.SOCKET_STATUS_BROKEN_REMOTELY; - HandleConnectionFailure(); } } @@ -480,8 +482,6 @@ namespace PepperDash.Core catch { Debug.Console(1, this, Debug.ErrorLogLevel.Error, "Stream write failed. Disconnected, closing"); - ClientStatus = SocketStatus.SOCKET_STATUS_BROKEN_REMOTELY; - HandleConnectionFailure(); } } diff --git a/Pepperdash Core/Pepperdash Core/Comm/GenericTcpIpClient.cs b/Pepperdash Core/Pepperdash Core/Comm/GenericTcpIpClient.cs index 5f09f36..0d5eb29 100644 --- a/Pepperdash Core/Pepperdash Core/Comm/GenericTcpIpClient.cs +++ b/Pepperdash Core/Pepperdash Core/Comm/GenericTcpIpClient.cs @@ -5,7 +5,6 @@ using System.Text; using System.Text.RegularExpressions; using Crestron.SimplSharp; using Crestron.SimplSharp.CrestronSockets; - using Newtonsoft.Json; using Newtonsoft.Json.Linq; @@ -17,6 +16,10 @@ namespace PepperDash.Core public class GenericTcpIpClient : Device, ISocketStatusWithStreamDebugging, IAutoReconnect { private const string SplusKey = "Uninitialized TcpIpClient"; + + /// + /// Enables debugging to console + /// public CommunicationStreamDebugging StreamDebugging { get; private set; } /// @@ -167,6 +170,9 @@ namespace PepperDash.Core get { return Client.ClientStatus == SocketStatus.SOCKET_STATUS_CONNECTED; } } + //Lock object to prevent simulatneous connect/disconnect operations + private readonly object connectLock = new object(); + CTimer RetryTimer; /// @@ -187,12 +193,7 @@ namespace PepperDash.Core RetryTimer = new CTimer(o => { - if (Client == null) - { - return; - } - - Client.ConnectToServerAsync(ConnectToServerCallback); + Reconnect(); }, Timeout.Infinite); CrestronEnvironment.ProgramStatusEventHandler += new ProgramStatusEventHandler(CrestronEnvironment_ProgramStatusEventHandler); @@ -212,12 +213,7 @@ namespace PepperDash.Core RetryTimer = new CTimer(o => { - if (Client == null) - { - return; - } - - Client.ConnectToServerAsync(ConnectToServerCallback); + Reconnect(); }, Timeout.Infinite); } @@ -234,12 +230,7 @@ namespace PepperDash.Core RetryTimer = new CTimer(o => { - if (Client == null) - { - return; - } - - Client.ConnectToServerAsync(ConnectToServerCallback); + Reconnect(); }, Timeout.Infinite); } @@ -259,7 +250,7 @@ namespace PepperDash.Core if (programEventType == eProgramStatusEventType.Stopping) { Debug.Console(1, this, "Program stopping. Closing connection"); - Disconnect(); + Deactivate(); } } @@ -269,6 +260,8 @@ namespace PepperDash.Core /// public override bool Deactivate() { + RetryTimer.Stop(); + RetryTimer.Dispose(); if (Client != null) { Client.SocketStatusChange -= this.Client_SocketStatusChange; @@ -282,9 +275,6 @@ namespace PepperDash.Core /// public void Connect() { - if (IsConnected) - DisconnectClient(); - if (string.IsNullOrEmpty(Hostname)) { Debug.Console(1, Debug.ErrorLogLevel.Warning, "GenericTcpIpClient '{0}': No address set", Key); @@ -298,32 +288,58 @@ namespace PepperDash.Core } } + lock(connectLock) + { + if (IsConnected) + { + Debug.Console(0, this, Debug.ErrorLogLevel.Warning, "Connection already connected. Exiting Connect()"); + } + else + { + //Stop retry timer if running + RetryTimer.Stop(); + + Client = new TCPClient(Hostname, Port, BufferSize); + Client.SocketStatusChange -= Client_SocketStatusChange; + Client.SocketStatusChange += Client_SocketStatusChange; + DisconnectCalledByUser = false; + + Client.ConnectToServerAsync(ConnectToServerCallback); + } + } + } + + private void Reconnect() + { if (Client == null) { - Client = new TCPClient(Hostname, Port, BufferSize); - Client.SocketStatusChange -= Client_SocketStatusChange; - Client.SocketStatusChange += Client_SocketStatusChange; + return; } - DisconnectCalledByUser = false; - - Client.ConnectToServerAsync(ConnectToServerCallback); // (null); - } + lock (connectLock) + { + if (IsConnected || DisconnectCalledByUser == true) + { + Debug.Console(0, this, Debug.ErrorLogLevel.Warning, "Reconnect no longer needed. Exiting Reconnect()"); + } + else + { + Client.ConnectToServerAsync(ConnectToServerCallback); + } + } + } /// /// Attempts to disconnect the client /// public void Disconnect() { - DisconnectCalledByUser = true; - - // Stop trying reconnects, if we are - RetryTimer.Stop(); - - if (Client != null) + lock (connectLock) { + DisconnectCalledByUser = true; + + // Stop trying reconnects, if we are + RetryTimer.Stop(); DisconnectClient(); - Client = null; - Debug.Console(1, this, "Disconnected"); } } @@ -335,7 +351,7 @@ namespace PepperDash.Core if (Client != null) { Debug.Console(1, this, "Disconnecting client"); - if(IsConnected) + if (IsConnected) Client.DisconnectFromServer(); } } @@ -347,7 +363,7 @@ namespace PepperDash.Core void ConnectToServerCallback(TCPClient c) { Debug.Console(1, this, "Server connection result: {0}", c.ClientStatus); - if (c.ClientStatus != SocketStatus.SOCKET_STATUS_CONNECTED && AutoReconnect) + if (c.ClientStatus != SocketStatus.SOCKET_STATUS_CONNECTED) WaitAndTryReconnect(); } @@ -356,16 +372,18 @@ namespace PepperDash.Core /// void WaitAndTryReconnect() { - DisconnectClient(); - - if (Client != null) + CrestronInvoke.BeginInvoke(o => { - Debug.Console(1, this, "Attempting reconnect, status={0}", Client.ClientStatus); - - if (!DisconnectCalledByUser) - RetryTimer.Reset(AutoReconnectIntervalMs); - } - + lock (connectLock) + { + if (!IsConnected && AutoReconnect && !DisconnectCalledByUser && Client != null) + { + DisconnectClient(); + Debug.Console(1, this, "Attempting reconnect, status={0}", Client.ClientStatus); + RetryTimer.Reset(AutoReconnectIntervalMs); + } + } + }); } /// @@ -398,7 +416,6 @@ namespace PepperDash.Core Debug.Console(0, this, "Received {1} characters of text: '{0}'", ComTextHelper.GetDebugText(str), str.Length); textHandler(this, new GenericCommMethodReceiveTextArgs(str)); - } } client.ReceiveDataAsync(Receive); @@ -416,8 +433,6 @@ namespace PepperDash.Core Debug.Console(0, this, "Sending {0} characters of text: '{1}'", text.Length, ComTextHelper.GetDebugText(text)); if(Client != null) Client.SendData(bytes, bytes.Length); - - } /// @@ -453,26 +468,18 @@ namespace PepperDash.Core void Client_SocketStatusChange(TCPClient client, SocketStatus clientSocketStatus) { Debug.Console(1, this, "Socket status change {0} ({1})", clientSocketStatus, ClientStatusText); - if (client.ClientStatus != SocketStatus.SOCKET_STATUS_CONNECTED && !DisconnectCalledByUser && AutoReconnect) + if (client.ClientStatus != SocketStatus.SOCKET_STATUS_CONNECTED) WaitAndTryReconnect(); - // Probably doesn't need to be a switch since all other cases were eliminated - switch (clientSocketStatus) - { - case SocketStatus.SOCKET_STATUS_CONNECTED: - Client.ReceiveDataAsync(Receive); - DisconnectCalledByUser = false; - break; - } + if(clientSocketStatus == SocketStatus.SOCKET_STATUS_CONNECTED) + { + Client.ReceiveDataAsync(Receive); + DisconnectCalledByUser = false; + } var handler = ConnectionChange; if (handler != null) ConnectionChange(this, new GenericSocketStatusChageEventArgs(this)); - - // Relay the event - //var handler = SocketStatusChange; - //if (handler != null) - // SocketStatusChange(this); } } diff --git a/Pepperdash Core/Pepperdash Core/Logging/Debug.cs b/Pepperdash Core/Pepperdash Core/Logging/Debug.cs index 211bc2d..d15f711 100644 --- a/Pepperdash Core/Pepperdash Core/Logging/Debug.cs +++ b/Pepperdash Core/Pepperdash Core/Logging/Debug.cs @@ -537,10 +537,6 @@ namespace PepperDash.Core //check for file at old path if (!File.Exists(oldFilePath)) { - Console(0, ErrorLogLevel.Notice, - String.Format( - @"Debug settings file not found at \nvram\debugSettings\program{0}. Attempting to use file at \user\debugSettings\program{0}", - InitialParametersClass.ApplicationNumber)); return; }