1
\$\begingroup\$

So I'm currently learning about networking between a client and a server and I decided to try to write my own client and server.

I implemented a way of receiving data and it goes as follows..

The server expects a packet that is structured like this.. [ packetSize(2bytes), [data] ], so the first two bytes is the entire packet size (size(2bytes) + data(X bytes)).

And my implementation receives all the data just fine, I just feel like it could be cleaner, by maybe using a BinaryReader or something similar, the goal here is to maybe get rid of all the Buffer.BlockCopy(.. or anything to make it cleaner, maybe even improving it.

class Connection
{
    private TcpClient _socket;

    private int _received;
    private int _packetSize = -1;
    private int bufferSize = 1024;
    private int offset = 0;
    private byte[] _buffer;
    private byte[] _packet;


    private BinaryReader reader;

    public Connection(TcpClient client)
    {
        _buffer = new byte[bufferSize];
        _socket = client;
        reader = new BinaryReader(_socket.GetStream());

        Console.WriteLine($"Client connected: {_socket.Client.RemoteEndPoint}");
        ProcessPackets();
    }

    public void ProcessPackets()
    {
        Task.Run(() =>
        {
            /* 
             * Packet Initializer
             * Receive The PacketSize
             * Instantiate the Packet byte[], giving it the correct size
             */

            _received = _socket.Client.Receive(_buffer, 0, 2, SocketFlags.None);
            _packetSize = BitConverter.ToInt16(_buffer, 0);
            _packet = new byte[_packetSize];

            /* Copy over the bytes that represent the size and update the offset. */
            Buffer.BlockCopy(_buffer, 0, _packet, 0, _received);
            offset = _received;

            /* Append data to the packet while there still is data to receive. */
            while (_received < _packetSize)
            {
                var delta = _packetSize - _received;
                if (delta <= bufferSize)
                {
                    _received += _socket.Client.Receive(_buffer, 0, delta, SocketFlags.None);
                    Buffer.BlockCopy(_buffer, 0, _packet, offset, _received - offset);
                }
                else
                {
                    _received += _socket.Client.Receive(_buffer, 0, bufferSize, SocketFlags.None);
                    Buffer.BlockCopy(_buffer, 0, _packet, offset, _received - offset);
                }

                offset += bufferSize;
            }

            //Reset the offset.
            offset = 0;

            /*junk*/
            var data = Encoding.ASCII.GetString(_packet.Skip(2).ToArray());
            Console.WriteLine($"Data received: {data}");

        });
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ What is _socket? Could you provide more code? \$\endgroup\$
    – aepot
    Jun 22, 2021 at 20:28
  • \$\begingroup\$ @aepot there we go :-) \$\endgroup\$
    – RileyDan
    Jun 22, 2021 at 20:37
  • \$\begingroup\$ Did I answer the question? \$\endgroup\$
    – aepot
    Aug 30, 2021 at 0:41

1 Answer 1

1
\$\begingroup\$

Future note: avoid making Code Review request for not finished yet solution.

Few notes:

  • What I'm really missing here: Asynchronous programming
  • There's no reason to stay in ASCII, use UTF8 instead. Also ASCII is subset of UTF-8.
  • You run receiving packet once and then nothing.
  • You aren't return data anywhere, why?
  • Buffer.BlockCopy is really fast but reading data from network directly to target array would look more efficient.
  • Avoid making fields (global variables) for single method. Make locals instead. If you think that declaring primitives e.g. int field once is more efficient than creating it once per loop iteration as local variable, then you're wrong.
  • Int16 is signed type. You can double the packet max size just changing it to UInt16.
  • There's no sense to store the data length in the received output data because array.Length will contain it.

Here's my try to play with.

class Connection
{
    private readonly TcpClient _client;
    private readonly NetworkStream _stream;
    private readonly Task _task;
    private readonly CancellationTokenSource _cts;

    private const int bufferSize = ushort.MaxValue; // 65536

    public Connection(TcpClient client)
    {
        _client = client;
        _stream = _client.GetStream();
        _cts = new CancellationTokenSource();
        _task = ReceiveAsync(_cts.Token);

        Console.WriteLine($"Client connected: {_client.Client.RemoteEndPoint}");
    }

    public async Task SendAsync(string message)
    {
        byte[] data = Encoding.UTF8.GetBytes(message);
        ushort size = (ushort)data.Length;
        await _stream.WriteAsync(BitConverter.GetBytes(size)).ConfigureAwait(false);
        await _stream.WriteAsync(data).ConfigureAwait(false);
    }

    private async Task ReceiveAsync(CancellationToken token)
    {
        byte[] buffer = new byte[bufferSize];
        try
        {
            while (true)
            {
                await _stream.ReadAsync(buffer.AsMemory(0, 2), token).ConfigureAwait(false);
                int size = BitConverter.ToUInt16(buffer, 0);
                int offset = 0;

                // normally there will be only one iteration of this loop but
                // ReadAsync doesn't guarantee that 'received' will always match
                // requested bytes amount
                while (offset < size) 
                {
                    int received = await _stream.ReadAsync(buffer.AsMemory(offset, size - offset), token).ConfigureAwait(false);
                    if (received == 0)
                    {
                        Console.WriteLine($"Client {_client.Client.RemoteEndPoint} disconnected.");
                        return;
                    }
                    offset += received;
                }

                // probably firing an event here would be helpful
                // byte[] output = buffer.AsSpan(0, size).ToArray();
                var data = Encoding.UTF8.GetString(buffer.AsSpan(0, size));
                Console.WriteLine($"Data received: {data}");
            }
        }
        catch (OperationCanceledException)
        {
            if (_client.Connected)
            {
                _stream.Close();
                Console.WriteLine($"Connection to {_client.Client.RemoteEndPoint} closed.");
            }
        }
        catch (Exception ex)
        {
            // Test the class a lot with closing the connection on both sides
            // I'm not sure how it will behave because I didn't test it
            Console.WriteLine($"{ex.GetType().Name}: {ex.Message}");
            throw;
        }
    }

    public void Close()
    {
        _cts.Cancel();
        _task.Wait();
    }
}
\$\endgroup\$

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.