2
\$\begingroup\$

I'm writing a TCP server using tasks and I wanted to know if there's something I can improve and how I am doing.

ConnectionHandler.cs:

public class ConnectionHandler
{
    private List<TClient> CLIENT_LIST;
    private TcpListener LISTENER;
    private const int PORT = 8080;
    private const int ACCEPT_DELAY = 75;

    public bool IsListening => LISTEN_FLAG;
    private bool LISTEN_FLAG = false;

    public ConnectionHandler()
    {
        LISTENER = new TcpListener(IPAddress.Any, PORT);
        CLIENT_LIST = new List<TClient>();
    }

    public void StartListeningAsync()
        => Task.Run((Action)StartListening);

    public void StartListening()
    {
        LISTEN_FLAG = true;
        LISTENER.Start();
        while (LISTEN_FLAG)
        {
            Task.Run(() => CreateConnectionAsync(LISTENER.AcceptSocket()));
            Thread.Sleep(ACCEPT_DELAY);
        ]
        LISTENER.Stop();
            CLIENT_LIST.Clear();
        }


    public void StopListening()
        => LISTEN_FLAG = false;

    private void CreateConnectionAsync(Socket connectionSocket)
        => Task.Run(() => CreateConnection(connectionSocket));

    private void CreateConnection(Socket connectionSocket)
    {
        TClient CLIENT = new TClient(connectionSocket);
        CLIENT.DisconnectEvent += new TClient.ClientEvent(DisconnectEventHanlder);
        CLIENT_LIST.Add(CLIENT);
    }

    private void DisconnectEventHanlder(TClient sender)
        => CLIENT_LIST.Remove(sender);
 }

Here I start to accept the incoming connections, then I create a TClientclass, add it to a List and add an event for the client disconnection.

TClient.cs:

public class TClient
{
    public delegate void Event();

    public delegate void DataEvent(IPacket data);
    public delegate void ClientEvent(TClient client);

    private int _SessionID;
    private bool _IsAvailable = true;
    private string _IPAdress;
    private Socket _connectionSocket;      

    public event ClientEvent DisconnectEvent;      
    public event DataEvent ProcessDataEvent;
    public event DataEvent SendDataEvent;

    public Socket ConnectionSocket => _connectionSocket;
    public string IPAdress => _IPAdress;
    public int SessionID => _SessionID;
    public bool IsAvailable => _IsAvailable;

    private PacketHandler PacketHandler;
    private PacketProcessor PacketProcessor;
    private ConnectionMaintenance Maintenance;

    public int ProcessedCorruptedPackets => PacketProcessor.CorruptedPackets;
    public int ReceivedCorruptedPackets => PacketHandler.CorruptedPackets;
    public int ReceivedData => PacketHandler.ReceivedData;
    public int ReceivedPackets => PacketHandler.ReceivedPackets;

    public TClient(Socket connection)
    {
        _SessionID = SessionFactory.GenerateUniqueSession();
        _connectionSocket = connection;
        _IPAdress = connection.RemoteEndPoint.ToString().Split(':')[0];
        PacketHandler = PacketHandler.CreateHandler(this);
        PacketProcessor = PacketProcessor.CreateProcessor(this);
        Maintenance = ConnectionMaintenance.CreateConnectionMatinance(this);           
    }   


    public void Disconnect()
    {
        _connectionSocket.Close();
        _IsAvailable = false;
        DisconnectEvent(this);            
    }

    public void ProcessData(IPacket data)
        => ProcessDataEvent(data);

    public void SendData(IPacket data)
       => SendDataEvent(data);

    public void KeepAlive() => 
        Maintenance.AliveTime = 0;        

  }

This is were I store the socket and the other connection information, such as the IP address, session ID, other classes, and the events for sending data, and processing received data.

PacketHandler.cs:

public class PacketHandler
{
    private const short MAX_DATA_LENGTH = 1024;
    private const short LOOP_DELAY = 9;

    public int ReceivedPackets => _receivedPackets;
    public int ReceivedData => _receivedData;
    public int CorruptedPackets => _corruptedPackets;

    private int _corruptedPackets;
    private int _receivedPackets;
    private int _receivedData;

    private TClient CLIENT;
    public static PacketHandler CreateHandler(TClient client) => new PacketHandler(client);
    private PacketHandler(TClient client)
    {
        CLIENT = client;
        CLIENT.SendDataEvent += new TClient.DataEvent(SendPacket);         
        Task.Run(() => ReceiveDataLoop(CLIENT));        
    }  
    private void ReceiveDataLoop(TClient CLIENT)
    {
        while (CLIENT.IsAvailable)
        {
            try
            {
                ReceivePacket();                   
            }
            catch
            {
                CLIENT.Disconnect();                   
            }
            Thread.Sleep(LOOP_DELAY);
        }            
    }
    private void SendPacket(IPacket data)
    {
        CLIENT.ConnectionSocket.Send(BitConverter.GetBytes((short)data.Length));
        CLIENT.ConnectionSocket.Send(data.PacketData);
    }        
    private void ReceivePacket()
    {
        byte[] lengthHeader = new byte[2];
        CLIENT.ConnectionSocket.Receive(lengthHeader);
        ushort DataLength = BitConverter.ToUInt16(lengthHeader, 0);
        byte[] PacketHeader = new byte[1];           
        CLIENT.ConnectionSocket.Receive(PacketHeader);
        byte[] data = new byte[DataLength];
        CLIENT.ConnectionSocket.Receive(data);        
        if (data.Length > MAX_DATA_LENGTH) _corruptedPackets++;
        else
        {
            CLIENT.ProcessData(new IPacket(PacketHeader[0], data));
            _receivedData += DataLength;
            _receivedPackets++;
        }
    }
   }
  }

Here is were the data is received and sent. First I receive Ushort that indicates the length of the data received, then a byte that indicates what will the program have to when it's processed. This data is then passed to an IPacket.

public struct IPacket
{
    private byte _packetHeader;
    private byte[] _packetData;

    public byte[] PacketData => _packetData;
    public byte PacketHeader => _packetHeader;
    public int Length => _packetData.Length;

    public IPacket(byte _header, byte[] _data)
    {
        _packetData = _data;
        _packetHeader = _header;
    }       
}

Then this is converted to an IMessage:

public enum Header
{
    UNKNOWN = 0,
    CORRUPTED = 1,
    KEEP_ALIVE = 2
}
public struct IMessage
{

    private IPacket basePacket;
    private Header _header;
    public string DataString => Encoding.UTF8.GetString(basePacket.PacketData);
    public Header Header => GetHeader();

    public IMessage(IPacket _basePacket)
    {
        basePacket = _basePacket;
        _header = Header.UNKNOWN;
    }
    private Header GetHeader()
    {
        if (_header != Header.UNKNOWN)
            return _header;
        if (basePacket.PacketHeader == 0)
            return Header.KEEP_ALIVE;
        else return Header.CORRUPTED;
    }
}

This is what will process the messages:

public class PacketProcessor
{
    public int CorruptedPackets => _corruptedPackets;       

    private int _corruptedPackets;
    private TClient CLIENT;

    public static PacketProcessor CreateProcessor(TClient client) => new PacketProcessor(client);
    private PacketProcessor(TClient client)
    {
        CLIENT = client;
        CLIENT.ProcessDataEvent += new TClient.DataEvent(ProcessMessage);
    }        
    private void ProcessMessage(IPacket Packet)
    {
        IMessage message = new IMessage(Packet);
        if (message.Header == Header.KEEP_ALIVE)
            CLIENT.KeepAlive();
        else if (message.Header == Header.CORRUPTED)            
            _corruptedPackets++;                   
    }
}

Then the connection is checked for exessive data flow, corrupted packets and no keep alive here:

public class ConnectionMaintenance
{
    public int AliveTime { get; set; }

    private const short LOOP_DELAY = 250;
    private const short MAX_DATA_PER_TICK = 960;
    private const short MAX_PACKETS_PER_TICK = 32;
    private const short MAX_CORRUPTED_PACKETS = 50;
    private const short MAX_ALIVE_MS = 15000; 

    private TClient CLIENT;
    public static ConnectionMaintenance CreateConnectionMatinance(TClient client) => new ConnectionMaintenance(client);

    private ConnectionMaintenance(TClient client)
    {
        CLIENT = client;
        new Task(MaintenanceLoop).Start();
    }

    private void MaintenanceLoop()
    {                     
        int lastReceivedData = 0;
        int lastReceivedPackets = 0;                   
        while (CLIENT.IsAvailable)
        {
            if (CLIENT.ProcessedCorruptedPackets >= MAX_CORRUPTED_PACKETS
            || CLIENT.ReceivedCorruptedPackets >= MAX_CORRUPTED_PACKETS)
                CLIENT.Disconnect();
            else if ((CLIENT.ReceivedData - lastReceivedData) >= MAX_DATA_PER_TICK)
                CLIENT.Disconnect();
            else if ((CLIENT.ReceivedPackets - lastReceivedPackets) >= MAX_PACKETS_PER_TICK)
                CLIENT.Disconnect();
            else if (AliveTime >= MAX_ALIVE_MS)
                CLIENT.Disconnect();
            else
            {
                AliveTime += LOOP_DELAY;
                lastReceivedData = CLIENT.ReceivedData;
                lastReceivedPackets = CLIENT.ReceivedPackets;
            }
            Thread.Sleep(LOOP_DELAY);          
        }
    }

}

I am worried about the structure of the program. Should I be using the async methods provided by the TCP classes?

I'm trying to make everything like "modules" so if I can update the code without affecting much each other "module". Then I'm using a lot of private variables with public get accesors. Is this worth it? Or should I just use public variables?

As I'm trying to keep things "modular", it doesn't feel right, since everything is dependent on TClient and the other classes are interconnected through it.

\$\endgroup\$
1
\$\begingroup\$

Names Names Names

Members

We don't use UPPER_CASE in C# at all. Ok, I could live with UPPER_CASE const names but other members or even function parameters are a no-go. To me it looks like everything was a constant. It's very hard to read.

You are also not consistant because some members have almost correct names like _SessionID that actaully should be _sessionId.

Classes

We don't use the T prefix for type names. They are reserved for generic placeholders like Client<T>.

Structs

We don't use the I prefix for struct names. We use it only for interface names like IEnumerable.


LISTEN_FLAG

Try to name your variables so that they describe the class/object the are in. In this case it would be more appropriate to call it IsListening.

\$\endgroup\$
  • \$\begingroup\$ Thanks for asking. My naming is really bad, for TClient I don't know what to call it, if I just call it "Client" then what do I call the variable? Similar thing happens with private variables ("_SessionID"), should I use the name of the publig adding _ or should I call it something different? \$\endgroup\$ – null Aug 16 '16 at 16:54

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.