Async TCP connection that handles reading and writing

Question

My situation is as such:

• I must connect to a server via TCP, at the end of the session (several hours), they will close the connection - throwing an exception that should be gracefully handled
• I need to both send and receive messages, they are delineated by '\n'
• Per the specs if the server doesn't send a message within 2 minutes, I should attempt to reconnect (it should be sending heartbeats)
• This code works, shut down is NOT graceful at the moment though

However, I really feel this code is sub optimal. For starters we have a large while loop, that then waits for a socket read (with the timeout set to 2 minutes).

Additionally the way I parse the message feels clunky. The entire if (obj is X) else if (obj is y) seems verbose. I was looking for a way to do method overloading, but as all my messages inherit from an abstract base class, it seems method resolution can't find the instantiated sub type.

I feel as if there should be some way to say:

1. Set up the network connection parameters, ip, port
2. Set up up a simple handler that triggers the appropriate method on receiving (rather than my while loop with .Wait)
3. A cleaner way to handle shutting down

Essentially, I feel as though I have perhaps not found the highest level .Net library / class for handling this connection and feel as though perhaps I've chosen too low level options and am forced to implement some things that seem like they should be "solved problems".

Pick the bones clean:

using Microsoft.Extensions.Logging;
using System;
using System.Collections.Generic;
using System.Diagnostics.Eventing.Reader;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;
using Prescott.Messages;

namespace Prescott
{
    public class PrescottTcpClient: IDisposable
    {
        private Socket _server;
        private NetworkStream _networkStream;
        private StreamReader _streamReader;
        private StreamWriter _streamWriter;
        private readonly IPAddress _ipAddress;
        private readonly int _port;
        private readonly string _username;
        private readonly string _password;
        private const int Timeout = 120000; // milliseconds

        private bool _isDisposing = false;
        private bool _closeConnection = false;
        private long _openSequenceNumber;

        private readonly ILogger<PrescottTcpClient> _logger;

        public PrescottTcpClient(IPAddress ipAddress, int port, string username, string password, ILoggerFactory loggerFactory)
        {
            this._ipAddress = ipAddress;
            this._port = port;
            this._username = username;
            this._password = password;

            _logger = loggerFactory.CreateLogger<PrescottTcpClient>();
        }

        public void Start()
        {
            _closeConnection = false;

            var ipep = new IPEndPoint(_ipAddress, this._port);
            _server = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

            try
            {
                _logger.LogInformation("Connecting to {1}:{2}", DateTime.Now, _ipAddress, _port);
                _server.Connect(ipep);
            }
            catch (SocketException ex)
            {
                // TODO Do we want to retry a few times? Failover?
                _logger.LogError("Unable to connect to server. Ex {0}", ex.ToString());
                return;
            }

            _networkStream = new NetworkStream(_server);
            _streamReader = new StreamReader(_networkStream);
            _streamWriter = new StreamWriter(_networkStream);

            Login();
            EventLoop();
        }

        private void Login()
        {
            var login = $"loginmessage";
            _streamWriter.WriteLine(login);
            _streamWriter.Flush();
            _logger.LogTrace(login);
        }

        // If the message ID's skip, we want to request missed messages
        private void GetSnapshot(long priorSequenceNumber, long currentSequenceNumber)
        {   
            var snapshot = $"requestMissedMessages";
            _streamWriter.WriteLine(snapshot);
            _streamWriter.Flush();
            _logger.LogInformation(snapshot);
        }

        private void EventLoop()
        {
            var reconnect = false;

            try
            {
                while (!_closeConnection)
                {
                    var data = _streamReader.ReadLineAsync();
                    var hasMessage = data.Wait(Timeout);

                    if (hasMessage)
                    {
                        try
                        {
                            var message = Mapper.ToPrescottMessage(data.Result);

                            if (message is Message1)
                                HandleMessage((Message1)message);
                            else if (message is Message2)
                                HandleMessage((Message2)message);
                            /*
                             * More message types here
                             */

                            MessageRecieved?.Invoke(this, message);
                        }
                        catch (Exception ex)
                        {
                            _logger.LogError(ex.ToString());
                        }
                    }
                    else
                    {
                        //Attempt to reconnect, because the timeout was hit.
                        //TODO Reconnect limit maybe?
                        _logger.LogInformation("Server did not respond, reconnecting");
                        _closeConnection = true;
                        reconnect = true;
                    }
                }
            }
            catch (Exception ex)
            {
                //TODO Handle Server force shut downs
                _logger.LogError(ex.ToString());
            }

            if (reconnect)
                Restart();
        }

        public event EventHandler<MsrbMessage> MessageRecieved;

        #region Message Handlers

        private void HandleMessage(Message1 message)
        {
            // do stuff
        }

        private void HandleMessage(Message2 message)
        {
            // do stuff
        }

        /* 
         * 5 or 6 different HandleMessage Methods here, all messages
         * inherit from abstract base "message"
         */

        #endregion

        public void Dispose()
        {
            if (!_isDisposing)
                Close();
        }

        public void Restart()
        {
            Close();
            Start();
        }

        public void Close()
        {
            _closeConnection = true;

            //_server?.Shutdown(SocketShutdown.Both); // This might displose the streams and network..
            _server?.Dispose();
            _streamReader?.Dispose();
            _streamWriter?.Dispose();
            _networkStream?.Dispose();
        }

    }
}

Answer 1

PrescottTcpClient should not be handling messages; the messages should know how to handle themselves.

That is, the abstract Message class should define an abstract Handle() method, and the logic contained in HandleMessage(Message1 message) should be moved into Handle() in the Message1 class.

Then, your if (message is Message1) ... code block simply becomes:

message.Handle();

(You might want to consider another name, like "Execute" or "Process" or "Respond".)

Now, your message handling code probably changes some instance variables of PrescottTcpClient. In that case, you can either pass this as an argument to the Handle() method, or, better yet, create a SessionState object that contains only the information that the message handling code needs to read from or write to.

Advantages:

• PrescottTcpClient can be focused on networking-related concerns, and "business logic" can stay in the SessionState and Message objects.
• You can add new messages without changing PrescottTcpClient.
• You can write unit tests that set up a SessionState object, pass it to a message object, and see that it did the right things: all without invoking any networking code.