4
\$\begingroup\$

I'm writing an asynchronous TCP server and client for a game in C# and in particular getting sending right is a bit tricky because calling BeginSend() before EndSend() for a previous send has been called must be avoided. Also I possibly have to deal with partial sends - the documentation on BeginSend() and EndSend() is in conflict on this so I'm staying on the safe side there. The following code works pretty well but I'm not totally happy about using lock().

The class encapsulates a TCP connection and allows message-based communication.

If anyone can find some issues with this - or give me an idea how to make this lock-free I would be happy :) I had previously posted an edited snippet to focus just on the sending part but was lectured that actual working code is required for reviews so this is the real deal.

While preparing the code I did find that having this class be abstract might be a bad smell and I should probably just move to composition instead of inheritance for code reuse and make OnMessageReceived() and OnClose() events instead of abstract methods.

using Engine.Shared.Logging;
using Engine.Shared.Messaging;
using System;
using System.Collections.Generic;
using System.Net.Sockets;

namespace Engine.Shared.Net
{
    /// <summary>
    /// ConnectionHandler handles a single connection to a server or client.
    /// </summary>
    public abstract class ConnectionBase : IDisposable
    {
        protected Socket Socket { get; set; }
        private bool _started = false;


        const int _bufferSize = 8192;
        private byte[] _receiveBuffer = new byte[_bufferSize];
        private object _lock = new object();

        private byte _highLengthByte;
        private byte[] _messageData;
        private int _readMessageOffset;
        private int _messageLength;
        ReadState _readState;

        private Queue<NetworkMessage> _sendQueue = new Queue<NetworkMessage>();
        private NetworkMessage _currentSendMessage;
        private int _currentSendOffset;
        private bool _sending;

        private bool disposed = false;


        /// <summary>
        /// 
        /// </summary>
        /// <param name="socket">A connected socket</param>
        public ConnectionBase(Socket socket)
        {
            Socket = socket;
        }


        protected ConnectionBase()
        { }


        public void Start()
        {
            Socket.NoDelay = true;
            _started = true;
            ReceiveNext();
        }

        public void SendMessage(NetworkMessage message)
        {
            lock (_sendQueue)
            {
                _sendQueue.Enqueue(message);
                TrySendNext();
            }
        }

        public virtual void Close(bool notify)
        {
            if (!_started)
                return;
            _started = false;

            try
            {
                Socket.Shutdown(SocketShutdown.Both);
            }
            catch (Exception ex)
            {
                Logger.Log(ex);
            }
            //try 
            //{
            //    Socket.Disconnect(false);
            //}
            //catch (Exception ex)
            //{
            //    Logger.Log(ex);
            //}
            try
            {
                Socket.Close();
            }
            catch (Exception ex)
            {
                Logger.Log(ex);
            }
            if (notify)
                OnClose();

        }

        protected abstract void OnMessageReceived(NetworkMessage message);

        protected abstract void OnClose();

        private void ReceiveNext()
        {
            if (!_started)
                return;
            try
            {
                Socket.BeginReceive(_receiveBuffer, 0, _bufferSize, 0, new AsyncCallback(ReceiveCallback), null);
            }
            catch (Exception)// ex)
            {
                Close(true);
            }
        }

        private void ReceiveCallback(IAsyncResult ar)
        {
            int read = 0;
            try
            {
                read = Socket.EndReceive(ar);
                if (read > 0)
                {
                    ProcessReadBytes(read);
                }
                else
                {
                    Close(true);
                }
            }
            catch (Exception ex)
            {
                Logger.Log(ex);
                Close(true);
            }
            finally
            {
                if (read > 0)
                {
                    ReceiveNext();
                }
            }
        }


        /// <summary>
        /// Process read bytes and emit messages whenever one completes
        /// </summary>
        /// <param name="read"></param>
        private void ProcessReadBytes(int read)
        {
            //Logger.Log("ProcessReadBytes({0}) Thread={1}", read, Thread.CurrentThread.Name);
            int offset = 0;
            while (offset < read)
            {
                //ad-hoc statemachine
                switch (_readState)
                {
                    case ReadState.ReadFirstLengthByte:
                        _highLengthByte = _receiveBuffer[offset++];
                        _readState = ReadState.ReadSecondLengthByte;
                        _readMessageOffset++;
                        break;
                    case ReadState.ReadSecondLengthByte:
                        byte lowLengthByte = _receiveBuffer[offset++];
                        _messageLength = (_highLengthByte << 8) | lowLengthByte;
                        _messageData = new byte[_messageLength];
                        _messageData[0] = _highLengthByte;
                        _messageData[1] = lowLengthByte;
                        _readMessageOffset++;
                        _readState = ReadState.ReadPayload;
                        //Logger.Log("MessageLength=" + _messageLength);
                        break;
                    case ReadState.ReadPayload:
                        int bytesToCopy = Math.Min(_messageLength - _readMessageOffset, read - offset);
                        Array.Copy(_receiveBuffer, offset, _messageData, _readMessageOffset, bytesToCopy);
                        _readMessageOffset += bytesToCopy;
                        offset += bytesToCopy;
                        if (_readMessageOffset == _messageLength)
                        {
                            NetworkMessage message = new NetworkMessage(_messageData);
                            //Logger.Log("Message received: " + (MessageCommandType)message.Command + ", Length = " + _messageLength);
                            OnMessageReceived(message);
                            _messageData = null;
                            _readMessageOffset = 0;
                            _readState = 0;
                        }
                        break;
                }

            }
            //Logger.Log("Finished reading");
        }


        private void TrySendNext()
        {
            if (!_started)
                return;
            if (_sending)//If sending is true then this method will be called from the send callback.
                return;
            if (_currentSendMessage == null)
            {
                if (_sendQueue.Count == 0)
                    return;
                _currentSendMessage = _sendQueue.Dequeue();
                _currentSendOffset = 0;
            }
            if (_currentSendMessage != null)
            {
                _sending = true;
                try
                {
                    Socket.BeginSend(
                        _currentSendMessage.Buffer,
                        _currentSendOffset,
                        _currentSendMessage.Length - _currentSendOffset,
                        SocketFlags.None,
                        new AsyncCallback(SendCallback),
                        null);
                }
                catch (Exception ex)
                {
                    Logger.Log(ex);
                    Close(true);
                }
            }

        }

        private void SendCallback(IAsyncResult ar)
        {
            try
            {
                int sent = Socket.EndSend(ar);
                lock (_sendQueue)
                {
                    _currentSendOffset += sent;
                    if (_currentSendOffset == _currentSendMessage.Length)
                        _currentSendMessage = null;

                    _sending = false;

                    TrySendNext();
                }
            }

            catch (Exception ex)
            {
                Logger.Log(ex);
                Close(true);
            }

        }



        // Implement IDisposable.
        // Do not make this method virtual.
        // A derived class should not be able to override this method.
        public void Dispose()
        {
            Dispose(true);
            // Take yourself off the Finalization queue 
            // to prevent finalization code for this object
            // from executing a second time.
            GC.SuppressFinalize(this);
        }

        // Dispose(bool disposing) executes in two distinct scenarios.
        // If disposing equals true, the method has been called directly
        // or indirectly by a user's code. Managed and unmanaged resources
        // can be disposed.
        // If disposing equals false, the method has been called by the 
        // runtime from inside the finalizer and you should not reference 
        // other objects. Only unmanaged resources can be disposed.
        protected virtual void Dispose(bool disposing)
        {
            // Check to see if Dispose has already been called.
            if (!this.disposed)
            {
                // If disposing equals true, dispose all managed 
                // and unmanaged resources.
                if (disposing)
                {
                    // Dispose managed resources.

                }

                // Release unmanaged resources. If disposing is false, 
                // only the following code is executed.
                try
                {
                    Socket.Close();
                }
                catch (Exception)
                { }
                // Note that this is not thread safe.
                // Another thread could start disposing the object
                // after the managed resources are disposed,
                // but before the disposed flag is set to true.
                // If thread safety is necessary, it must be
                // implemented by the client.

            }
            disposed = true;
        }

        // Use C# destructor syntax for finalization code.
        // This destructor will run only if the Dispose method 
        // does not get called.
        // It gives your base class the opportunity to finalize.
        // Do not provide destructors in types derived from this class.
        ~ConnectionBase()
        {
            // Do not re-create Dispose clean-up code here.
            // Calling Dispose(false) is optimal in terms of
            // readability and maintainability.
            Dispose(false);
        }

        private enum ReadState
        {
            ReadFirstLengthByte,
            ReadSecondLengthByte,
            ReadPayload
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

1) Don't implement so-called disposable pattern unless you actually have some unmanaged resource that you have to finalize. It bloats your code and makes the life of garbage collector harder. Socket is a managed class, you should not touch it in finalizer, it will finalize itself, if needed.

2) Start and Close is a weird pair of methods. Usually it is either Start/Stop or Open/Close. But that's subjective, I guess. :)

3) IMHO, callback-based API is somewhat outdated. Usually ...Async methods result in cleaner code and are easier to use.

4) Message parser (the state machine part of your code) can probably be extracted into stand-alone component.

5) Have you intentionally picked TCP protocol over, say, UDP? Often when people write client-server games they want UDP, as it is generally faster and is a better fit for realtime message-based communication. There are cases where TCP can be better, but it is hard to tell, because you did not say, what type of game you are writing. So I am mentioning this just in case, to make sure you gave it some thought.

6) You should probably extract interface from this class and work with abstraction. Both for unit testing and in case you will switch to different protocol in the future.

7) Overall your implementation looks good. I don't see any obvious problems with synchronization, and I agree with you that abstract methods should probably be refactored into events.

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking your time to answer this:) All of your points gave me something to think about for sure. 1) I believe I should still implement disposable to make sure Socket object is disposed?, so I guess I should just remove the finalizer? 5) TCP vs UDP is a hard choice and I'm not sure I've made the right one. This is an MMORPG type game (minus the massive part). I know UDP is better for latency but guaranteed delivery means that I can send a lot less data because I know all previous data has been received. \$\endgroup\$ – Jesper Apr 20 '18 at 12:23
  • \$\begingroup\$ @Jesper no problem. 1) Yes. You also won't need a disposing flag. 5) For MMORPG-type games I think the best approach is to use both UDP and TCP. UDP - for receiving world state updates (such as, say positions of other players) where latency is more important. And TCP - for sending player commands (movement commands or skill use, for example) where guaranteed delivery is more important. But really, it is not something that you should bother with early. You can always come back to it, when your game is finished. Just make sure you design your code in away, that allows you to do so. \$\endgroup\$ – Nikita B Apr 20 '18 at 12:47
  • \$\begingroup\$ @Jesper, here is some good links, that can help you decide what works best for you: stackoverflow.com/a/32236908/1386995 \$\endgroup\$ – Nikita B Apr 20 '18 at 12:51
  • \$\begingroup\$ Yeah I have considered UDP or reliable UDP but came to the same conclusion. TCP is so much easier and the less code it takes to make version 1 the less effort it takes to change it. Either way if it's good enough for Diablo and Path of Exile it's probably good enough for me too:) \$\endgroup\$ – Jesper Apr 20 '18 at 15:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.