GameServer SocketListener + PlayerManagement

Question

Today I was posting a question to get a bit of information on how to optimize my code for maximum performance. I'm a beginner to C# (in a sense) and I don't really know as much as all of you other developers on this site.

I'm looking for just any improvements that you think I should make to write better quality code for higher performance, I'm not saying the performance is bad but what I have mostly learnt is there is always room for improved with code.

So, what I am posting is a Socket Listener class which listens for players connecting to the game server. When they have connected the player gets a new instance of 'PlayerConnection' with their socket and added to the PlayerManager's dictionary of players.

The purpose for this is each player will send and receive data to the server. We do it in packets and each packet has a unique identifier.

As of now I have only coded the receiving from Player side of things, I am yet to code sending data to the Player. You'll see I have coded something to do with sending because I have to first of all send a crossdomain policy but I haven't implemented a proper system for sending packets to the player from the server yet.

I've included a fair amount of detail, the rest is pretty much self explanatory or I've added comments in certain places.

ServerListener:

public class SocketListener : IDisposable, IPandaClass
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    private readonly Socket _listener;

    public SocketListener()
    {
        _listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    }

    public void Load()
    {
        var panda = Program.GetPandaServer();
        var config = panda.GetHandler("configuration") as ConfigurationData;

        if (config == null)
        {
            return;
        }

        var socketPort = config.GetValueByKeyInt("game.socket.port");
        var socketBacklog = config.GetValueByKeyInt("game.socket.backlog");

        _listener.Bind(new IPEndPoint(IPAddress.Any, socketPort));
        _listener.Listen(socketBacklog);
        _listener.BeginAccept(OnAccept, _listener);
    }

    private void OnAccept(IAsyncResult asyncResult)
    {
        try
        {
            if (_listener == null)
            {
                return;
            }

            var server = (Socket)asyncResult.AsyncState;
            var client = server.EndAccept(asyncResult);

            var player = new PlayerConnection(client);

            var panda = Program.GetPandaServer();
            var pandaBase = panda.GetHandler("base") as BaseHandler;

            if (pandaBase == null)
            {
                return;
            }

            var playerHandler = pandaBase.GetHandler("player") as PlayerHandler;

            if (playerHandler != null && !playerHandler.TryAddPlayer(player))
            {
                Logger.Error($"Failed to register players connection on: {client.RemoteEndPoint}");
            }
        }
        catch (SocketException socketException)
        {
            Logger.Error(socketException, $"Failed to accept socket connection: {socketException.Message}");
        }
        finally
        {
            _listener?.BeginAccept(OnAccept, _listener);
        }
    }

    public void Dispose()
    {
        _listener.Shutdown(SocketShutdown.Both);
        _listener = null;
    }

PlayerManager: There will be many more methods like GetPlayer DisconnectPlayer MassPacket and more once finished.

internal class PlayerHandler : IPandaClass
{
    private readonly ConcurrentDictionary<int, PlayerConnection> _players;

    public PlayerHandler()
    {
        _players = new ConcurrentDictionary<int, PlayerConnection>();
    }

    public bool TryAddPlayer(PlayerConnection player)
    {
        return _players.TryAdd(_players.Count, player);
    }
}

PlayerConnection:

public sealed class PlayerConnection : PlayerData, IDisposable
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    private readonly Socket _playerSocket;
    private readonly byte[] _buffer;

    private bool _halfDataReceived;
    private byte[] _halfData;
    public bool _decrypted;

    public PlayerConnection(Socket socket)
    {
        _playerSocket = socket;
        _buffer = new byte[8192];

        try
        {
            _playerSocket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, OnReceivedData, _playerSocket);
        }
        catch (Exception exception)
        {
            Logger.Error(exception, "Failed to start receiving on players socket.");
        }
    }

    private void OnReceivedData(IAsyncResult asyncResult)
    {
        try
        {
            var bytesReceived = _playerSocket.EndReceive(asyncResult);

            if (bytesReceived == 0)
            {
                Dispose();
                return;
            }

            var packet = new byte[bytesReceived];
            Array.Copy(_buffer, packet, bytesReceived);
            ProcessReceivedData(packet);
        }
        catch (SocketException socketException)
        {
            Logger.Error(socketException, "Failed while receiving data from players socket.");
        }
        finally
        {
            _playerSocket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, OnReceivedData, _playerSocket);
        }
    }

    private void ProcessReceivedData(byte[] receivedBytes)
    {
        if (!_decrypted)
        {
            // todo: decrypt here
            _decrypted = true;
        }

        var packetId = receivedBytes[0];

        if (packetId == 67)
        {
            return;
        }

        if (_halfDataReceived)
        {
            var fullDataRcv = new byte[_halfData.Length + receivedBytes.Length];

            Buffer.BlockCopy(_halfData, 0, fullDataRcv, 0, _halfData.Length);
            Buffer.BlockCopy(receivedBytes, 0, fullDataRcv, _halfData.Length, receivedBytes.Length);

            _halfDataReceived = false;

            ProcessReceivedData(fullDataRcv); // todo: repeat now we have the combined array
            return;
        }

        if (packetId == 60)
        {
            const string crossDomainPolicy = "<?xml version=\"1.0\"?>\r\n"
                                             + "<!DOCTYPE cross-domain-policy SYSTEM \"/xml/dtds/cross-domain-policy.dtd\">\r\n"
                                             + "<cross-domain-policy>\r\n"
                                             + "<site-control permitted-cross-domain-policies=\"master-only\"/>\r\n"
                                             + "<allow-access-from domain=\"*\" to-ports=\"*\" />\r\n"
                                             + "</cross-domain-policy>\x0";

            SendString(crossDomainPolicy);
        }
        else
        {
            using (var reader = new BinaryReader(new MemoryStream(receivedBytes)))
            {
                if (receivedBytes.Length < 4)
                {
                    return;
                }

                var packetLength = PacketUtilities.DecodeInt32(reader.ReadBytes(4));

                if (reader.BaseStream.Length - 4 < packetLength)
                {
                    _halfData = receivedBytes;
                    _halfDataReceived = true;
                    return;
                }

                if (packetLength < 0 || packetLength > 5120)
                {
                    return;
                }

                var packetBytes = reader.ReadBytes(packetLength);

                using (var binaryReader = new BinaryReader(new MemoryStream(packetBytes)))
                {
                    var packetHeader = PacketUtilities.DecodeInt16(binaryReader.ReadBytes(2));
                    var packetBodyBytes = new byte[packetBytes.Length - 2];

                    Buffer.BlockCopy(packetBytes, 2, packetBodyBytes, 0, packetBytes.Length - 2);

                    // now we handle the packet: packetHeader and packetBodyBytes

                    _decrypted = false;
                }

                if (reader.BaseStream.Length - 4 <= packetLength)
                {
                    return;
                }

                var extra = new byte[reader.BaseStream.Length - reader.BaseStream.Position];
                Buffer.BlockCopy(receivedBytes, (int)reader.BaseStream.Position, extra, 0, (int)(reader.BaseStream.Length - reader.BaseStream.Position));

                _decrypted = true;

                ProcessReceivedData(extra);
            }
        }
    }

    private void SendString(string data)
    {
        SendData(Encoding.UTF8.GetBytes(data));
    }

    private void SendData(byte[] data)
    {
        try
        {
            _playerSocket.BeginSend(data, 0, data.Length, 0, OnSend, null);
        }
        catch (SocketException socketException)
        {
            Logger.Error(socketException, "Error sending message to players socket.");
            Dispose();
        }
    }

    private void OnSend(IAsyncResult asyncResult)
    {
        try
        {
            if (_playerSocket == null)
            {
                return;
            }

            _playerSocket.EndSend(asyncResult);
        }
        catch (SocketException)
        {
            Dispose();
        }
    }

    public void Dispose()
    {
        if (_playerSocket == null || !_playerSocket.Connected)
        {
            return;
        }

        _playerSocket.Shutdown(SocketShutdown.Both);
        _playerSocket.Close();
    }
}

I'll most the other methods I used (Utility methods) below also.

PacketUtilities:

internal static int DecodeInt32(byte[] bytes)
{
    if ((bytes[0] | bytes[1] | bytes[2] | bytes[3]) < 0)
    {
        return -1;
    }

    return (bytes[0] << 24) + (bytes[1] << 16) + (bytes[2] << 8) + bytes[3];
}

internal static int DecodeInt16(byte[] bytes)
{
    if ((bytes[0] | bytes[1]) < 0)
    {
        return -1;
    }

    return (bytes[0] << 8) + bytes[1];
}

Answer 1

The methods DecodeInt32 and DecodeInt16 are very confusing. The bitwise or (|) of 2 bytes can never give you something less than 0. Furthermore, bytes[0] << 24 can give you a negative number. I really have no idea what these methods are meant to achieve.

---

Also, your packetId seems to be included in the packetLength, which may create issues in the future. If the idea is that only packets with packetId == 0 should be read, then you should make this clearer, and use a method which doesn't incorporate the first byte when computing the packetLength.

---

if (packetId == 67)

What does 67 mean? 'Magic Numbers' like 67 and 60 are cryptic, and generally a maintainability nightmare. They often appear in multiple places, and changing the magic number means going through and changing all of them (or else the system breaks). Much better to provide a named constant, e.g.

public const int CrossDomainPolicyRequestPacketId = 60;

I don't know if that is a good name because I don't really know what it means. You need to give it a good name so that I do know what it means.

---

I don't think this check makes any sense:

if (config == null)
{
    return;
}

I think you really want to throw an exception here, complaining that no configuration was provided, otherwise someone calling this method will receive no feedback if it fails. At the very least this warrants logging, but exceptions are your friends because they prevent your code doing something wild and dangerous by plodding on despite being in an invalid/unexpected state.

---

I'm not going to review ProcessReceivedData and OnReceivedData methods in full because the whole system has a serious problem, and I'll just explain briefly why:

You cannot know how many bytes you will receive each time OnReceivedData is called: it could be 1, it could be a thousand. Your code has to cope with this somehow, and statements like the following immediately remove any hope of dependable networking:

if (receivedBytes.Length < 4)
{
    return;
}

This says "if I don't have a full packet, throw away what I do have": now you will never receive that package, or likely any other.

Similarly, an invalid packetLength is not cause for ignoring the packet: if you don't read that packet, then the stream becomes meaningless: you must handle this. If you can't work out how to handle it now, then die as loudly as possible (invalidate the PlaerConnection and throw an expectation), so that it can't come back to bite you, because if you do ever face an invalid packet, you will know about it straight away, instead of finding that the program keeps on running but none of the data makes any sense.

My guess is that _halfDataReceived is concerned with not forgetting a packet when you read it at the same time as another packet, but with networking a packet could end up being cut up any way imaginable, and you have to compensate by accumulating bytes until you have all the data that you need. This sort of code can appear to work when it is used locally (i.e. on a single machine), but all bets are off when you go beyond that.

---

Though it is using a ConcurrentDictionary, PlayerHandler.TryAddPlayer isn't thread-safe as I think you intended (though it lacks documentation explaining what it should do, so it's difficult to comment usefully). It's hard to know what it should do without more code, but if you want to support simultaneous calls to TryAddPlayer (as opposed to just using a ConcurrentDictionary for the purpose of allowing concurrent reads) then you need to deal with the case where both calls read the same value of _players.Count. Even if you don't want simultaneous adding, you need to consider how this will break when you try removing players. A classic way would be to use an incrementing id, which you access with the Interlocked.Increment method.

---

Public types and members always benefit from having inline documentation (///). It forces whoever wrote the method to make sure they understand what they want it to do, it ensures that the maintainer knows what it is meant to do, and it tells consumers how they can use it.

Answer 2

I'd rename OnAccept to something like BeginAccept_callback. You will mainly see the OnSomething naming convention for virtual functions. Case in point- System.Windows.Forms.Control is chock full of virtual functions where the base implementation is to raise an event. Likewise for the other callback handlers.