TCP client reconnection logic running on 30 machines

Question

I am working on a service that is supposed to run on a large installation 24/7. Once the software is deployed, I will most likely not be able to make any changes anymore. This is my first time working on a network application so I want to make sure I am not making any obvious mistakes.

60 instances of this program are supposed to run on 30 machines, meaning there are 2 instances of it running on each machine. It is used to keep all the client instances (running this code) connected to the controller that gives them updates when needed.

I am wondering if there are any optimizations you guys can recommend to me or if there are any other insufficiencies that catch your eyes.

public class ControllerConnectionService {

    public ControllerConnectionService() {
        Initialize();
    }

    private Timer _pingTimer;
    private SimpleTcpClient _tcpClient;

    public event EventHandler<DataModel> ContentTypeChanged;

    private void Initialize() {
        CreateTcpClient();
        Connect();

        var messageString = GetMessageString(MessageType.SendId, MainViewModel.Id.ToString());
        SendMessage(messageString);

        _pingTimer = new Timer {Interval = 2000};
        _pingTimer.Elapsed += PingTimerOnElapsed;
        _pingTimer.Start();
    }

    private void PingTimerOnElapsed(object sender, ElapsedEventArgs elapsedEventArgs) {
        if (!IsConnected()) Reconnect();
    }

    private bool IsConnected() {
        try {
            var pingBytes = Encoding.UTF8.GetBytes(GetMessageString(MessageType.Ping, string.Empty));
            _tcpClient.TcpClient.Client.Send(pingBytes);
            return true;
        }
        catch (Exception) {
            return false;
        }
    }

    private void CreateTcpClient() {
        _tcpClient = new SimpleTcpClient();
        _tcpClient.DataReceived += TcpClientOnDataReceived;
    }

    private void Connect() {
        try {
            _tcpClient.Connect(ControllerIp, ClientControllerPort);
        }
        catch (Exception) {
            Reconnect();
        }
    }


    private void Reconnect() {
        try {
            while (!IsConnected()) {
                Thread.Sleep(500);
                CreateTcpClient();

                _tcpClient.Connect(ControllerIp, ClientControllerPort);
            }
        }
        catch (Exception) {
            Reconnect();
        }
    }

    private void SendMessage(string message) {
        try {
            _tcpClient.TcpClient.Client.Send(Encoding.UTF8.GetBytes(message));
        }
        catch (Exception) {
            Reconnect();
        }
    }

    private void TcpClientOnDataReceived(object sender, Message message) {
        if (string.IsNullOrEmpty(message.MessageString))
            return;

        var messageParts = message.MessageString.Split(';');

        Enum.TryParse(messageParts[0], out MessageType messagetype);

        if (messagetype == MessageType.UpdateContentType) {
            Console.WriteLine($@"{DateStamp} Data received: {message.MessageString} ");
            var dataModel = JsonConvert.DeserializeObject<DataModel>(messageParts[1]);

            ContentTypeChanged?.Invoke(this, dataModel);
        }
    }
}

If you require any additional information or code, feel free to ask.

Answer 1

1. Being that both Timer and SimpleTcpClient implement IDisposable, your class should also implement IDisposable and follow the disposable pattern.

2. Do not call Initialize from your constructor. Constructors are for initializing state, not starting processing. Your calling code should call the constructor and then call Initialize.

3. It would be handy if you included GetMessageString in your code for review.

4. var pingBytes = Encoding.UTF8.GetBytes(GetMessageString(MessageType.Ping, string.Empty)); is calculated in every call to IsConnected. This seems like a pretty static value that should only be done once.

5. The Reconnect method calls itself in an exceptional condition. If the condition doesn't clear, it will eventually cause a Stack Overflow. You should only retry reconnecting some number of times and after that let the caller handle the exception.

With all this in mind, here is some code. I fudged some of the things that don't exist in your original post so it could compile. So ignore those.

public class ControllerConnectionService : IDisposable
{
    private static readonly byte[] _pingBytes = Encoding.UTF8.GetBytes(GetMessageString(MessageType.Ping, string.Empty));

    private SimpleTcpClient _tcpClient;

    private Timer _pingTimer;

    public ControllerConnectionService()
    {
        Create();
    }

    public void Dispose()
    {
        Close();
    }

    public event EventHandler<DataModel> ContentTypeChanged;

    private MainViewModel MainViewModel { get; } = new MainViewModel { Id = 1 };
    private int ClientControllerPort { get; } = 8080;

    private string ControllerIp { get; } = "127.0.0.1";

    private static DateTime DateStamp => DateTime.UtcNow;

    private void Create()
    {
        if (_tcpClient == null)
        {
            _tcpClient = new SimpleTcpClient();
            _tcpClient.DataReceived += TcpClientOnDataReceived;
        }

        if (_pingTimer == null)
        {
            _pingTimer = new Timer { Interval = 2000 };
            _pingTimer.Elapsed += PingTimerOnElapsed;
        }
    }

    private void Close()
    {
        if (_pingTimer != null)
        {
            _pingTimer.Elapsed -= PingTimerOnElapsed;
            _pingTimer.Dispose();
            _pingTimer = null;
        }

        if (_tcpClient != null)
        {
            _tcpClient.DataReceived -= TcpClientOnDataReceived;
            _tcpClient.Dispose();
            _tcpClient = null;
        }
    }

    public void Initialize()
    {
        Connect();

        var messageString = GetMessageString(MessageType.SendId, MainViewModel.Id.ToString());
        SendMessage(messageString);

        _pingTimer.Start();
    }

    private void PingTimerOnElapsed(object sender, ElapsedEventArgs elapsedEventArgs)
    {
        if (!IsConnected()) Reconnect();
    }

    private bool IsConnected()
    {
        try
        {
            _tcpClient.TcpClient.Client.Send(_pingBytes);
            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }

    private void Connect()
    {
        try
        {
            _tcpClient.Connect(ControllerIp, ClientControllerPort);
        }
        catch (Exception)
        {
            Reconnect();
        }
    }

    private void Reconnect()
    {
        try
        {
            while (!IsConnected())
            {
                Thread.Sleep(500);
                Close();
                Create();

                _tcpClient.Connect(ControllerIp, ClientControllerPort);
            }
        }
        catch (Exception)
        {
            // This is recursion and an exceptional condition above will cause a Stack Overflow.
            // Implement a "number of retries" before just throwing the exception and letting the
            // caller handle it.
            Reconnect();
        }
    }

    private void SendMessage(string message)
    {
        try
        {
            _tcpClient.TcpClient.Client.Send(Encoding.UTF8.GetBytes(message));
        }
        catch (Exception)
        {
            Reconnect();
        }
    }

    private void TcpClientOnDataReceived(object sender, Message message)
    {
        if (string.IsNullOrEmpty(message.MessageString))
            return;

        var messageParts = message.MessageString.Split(';');

        Enum.TryParse(messageParts[0], out MessageType messagetype);

        if (messagetype == MessageType.UpdateContentType)
        {
            Console.WriteLine($@"{DateStamp} Data received: {message.MessageString} ");
            var dataModel = JsonConvert.DeserializeObject<DataModel>(messageParts[1]);

            ContentTypeChanged?.Invoke(this, dataModel);
        }
    }

    private static string GetMessageString(MessageType messageType, string data)
    {
        return messageType + ":" + data;
    }
}

Answer 2

catch (Exception)
{
  Reconnect();
}

The same pattern can be found in almost every method. Are you not interested in what happened? No logging or anything? Just ignore the exception?

---

_pingTimer = new Timer { Interval = 2000 };
Thread.Sleep(500);

Hardcoded numbers? No settings? What if this won't work and you have to tune it? You'll have to update all instances!

---

Enum.TryParse(messageParts[0], out MessageType messagetype);

Why isn't this inside an if?

if (messagetype == MessageType.UpdateContentType)

This is the next line following the TryParse but this can go sideways if the UpdateContentType is the first value of the enum it might be returned as a default value and the flow will enter the if even if the massage part wasn't in the correct format.

---

private void TcpClientOnDataReceived(object sender, Message message)

You have try/catches for everything but this method. If something goes wrong here the application will crash.

---

60 instances of this program are supposed to run on 30 machines, meaning there are 2 instances of it running on each machine. It is used to keep all the client instances (running this code) connected to the controller that gives them updates when needed.

This sounds like too much. Why can't the client check for updates by itself?

Once the software is deployed, I will most likely not be able to make any changes anymore.

If you are not able to update it later then you should start writing all kinds of unit and integration tests to make sure it's bullet-proof but even then there will still be a bug somewhere... Not being able to deploy new versions is a terrible plan.

Answer 3

apart from what @t3chb0t has recommended, Reconnect should implement something like exponential back off or at least increase the sleep time also your IsConnected login can be improved TcpClient has a Connected property which you should check first. you only need to use Send to check for half dead connections which can either be handled with your Ping timer or use TCP KeepAlives. Also run the timer only when you are connected and PingMessage can be a pre-calculated const

Answer 4

With the Timer, I think your code isn't thread safe. It's possible that multiple calls to Reconnect() are called at the same time.

I would also log exceptions. If you ever need to debug a problem, it'll be much easier. You could ignore specific exception but I wouldn't ignore all of them.