Async socket client using TcpClient class C#

Question

I have implemented a socket client using TcpClient class. So I can send and receive data, everything works good. But I ask some of the guru's out there :) Is there something wrong with my implementation? maybe there is a better way of doing things. In particular, how do I handle disconnects? Is there some indicator (or maybe I can write one myself) that tells me that socket has disconnected?

I also have looked into async await features of Socket class, but can't wrap my head around "SocketAsyncEventArgs", why is it there in the first place. Why cant i just: await Client.SendAsync("data"); ?

public class Client
{
    private TcpClient tcpClient;

    public void Initialize(string ip, int port)
    {
        try
        {
            tcpClient = new TcpClient(ip, port);

            if (tcpClient.Connected)
                Console.WriteLine("Connected to: {0}:{1}", ip, port);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
            Initialize(ip, port);
        }
    }

    public void BeginRead()
    {
        var buffer = new byte[4096];
        var ns = tcpClient.GetStream();
        ns.BeginRead(buffer, 0, buffer.Length, EndRead, buffer);
    }

    public void EndRead(IAsyncResult result)
    {
        var buffer = (byte[])result.AsyncState;
        var ns = tcpClient.GetStream();
        var bytesAvailable = ns.EndRead(result);

        Console.WriteLine(Encoding.ASCII.GetString(buffer, 0, bytesAvailable));
        BeginRead();
    }

    public void BeginSend(string xml)
    {
        var bytes = Encoding.ASCII.GetBytes(xml);
        var ns = tcpClient.GetStream();
        ns.BeginWrite(bytes, 0, bytes.Length, EndSend, bytes);
    }

    public void EndSend(IAsyncResult result)
    {
        var bytes = (byte[])result.AsyncState;
        Console.WriteLine("Sent  {0} bytes to server.", bytes.Length);
        Console.WriteLine("Sent: {0}", Encoding.ASCII.GetString(bytes));
    }
}

And the usage:

static void Main(string[] args)
{
    var client = new Client();
    client.Initialize("127.0.0.1", 8778);

    client.BeginRead();
    client.BeginSend("<Names><Name>John</Name></Names>");

    Console.ReadLine();
}

Hatchet

Isn't this question more suited for codereview.stackexchange.com ? – Solder

Answer 1

Okay it took me 10 seconds to find biggest issue you could've done :

public void BeginRead()
{
    var buffer = new byte[4096];
    var ns = tcpClient.GetStream();
    ns.BeginRead(buffer, 0, buffer.Length, EndRead, buffer);
}

But don't worry that's why we are on SO.

---

First let me explain why it's such a big issue.

Assume that you're sending message which is 4097 bytes long. Your buffer can only accept 4096 bytes meaning that you cannot pack whole message into this buffer.

Assume you're sending message which is 12 bytes long. You're still allocating 4096 bytes in your memory just to store 12 bytes.

How to deal with this?

Every time you work with networking you should consider making some kind of protocol ( some people call it message framing, but it's just a protocol ) that will help you to identify incomming package.

Example of a protocol could be :

[1B = type of message][4B = length][XB = message]
- where X == BitConvert.ToInt32(length);

• Receiver:

  byte messageType = (byte)netStream.ReadByte();
  byte[] lengthBuffer = new byte[sizeof(int)];
  int recv = netStream.Read(lengthBuffer, 0, lengthBuffer.Length);
  if(recv == sizeof(int))
  {
      int messageLen = BitConverter.ToInt32(lengthBuffer, 0);
      byte[] messageBuffer = new byte[messageLen];
      recv = netStream.Read(messageBuffer, 0, messageBuffer.Length);
      if(recv == messageLen)
      {
          // messageBuffer contains your whole message ...
      }
  }
  

• Sender:

  byte messageType = (1 << 3); // assume that 0000 1000 would be XML
  byte[] message = Encoding.ASCII.GetBytes(xml);
  byte[] length = BitConverter.GetBytes(message.Length);
  byte[] buffer = new byte[sizeof(int) + message.Length + 1];
  buffer[0] = messageType;
  for(int i = 0; i < sizeof(int); i++)
  {
      buffer[i + 1] = length[i];
  }
  for(int i = 0; i < message.Length; i++)
  {
      buffer[i + 1 + sizeof(int)] = message[i];
  }
  netStream.Write(buffer);
  

---

Rest of your code looks okay. But in my opinion using asynchronous operations in your case is just useless. You can do the same with synchronous calls.

Sandarac

As the code could go almost anywhere from here, why do you suggest avoiding the asynchronous operations? Are there some clear benefits and costs to making such a decision at this stage of development? – Shin

Your solution of allocating a new buffer every time you want to read is extremely inefficient. 4K of memory is cheap and abundant. CPU cycles should not be wasted for that. – Almondeyed

Answer 2

It's hard to answer because theres no exact question here but more some kind of code review. But still some hints:

• Your connect mechanism seems wrong. I don't think TcpClient.Connected will block until the connection was established. So it will often just fail as the connection is in progress and then you start all over again. You should switch to using a blocking or async Connect method.
• SocketAsyncEventArgs is a mechanism for high performance async data transmission. There's rarely a need for it. You should just ignore it
• If you want to send data asynchronously you should use the Async methods which return a Task, because these can be easily combined with async/await.
• The APM model (BeginXYZ/EndXYZ) is kind of deprecated, you shouldn't use it anymore in new code. One issue with it is that sometimes the End method is called synchronously inside the Begin method which can lead to surprising behavior. If this is not the case the completion callback will be performed from a random thread on the ThreadPool. This is also often not what you want. The TPL methods avoid this.
• For your simple use case the blocking methods are also totally fine and do not come with the complexity of the various async methods.

The reading side of the code with TPL methods (untested):

public async Task Initialize(string ip, int port)
{
    tcpClient = new TcpClient;
    await tcpClient.ConnectAsync(ip, port);

    Console.WriteLine("Connected to: {0}:{1}", ip, port);
}

public async Task Read()
{
    var buffer = new byte[4096];
    var ns = tcpClient.GetStream();
    while (true)
    {
        var bytesRead = await ns.ReadAsync(buffer, 0, buffer.Length);
        if (bytesRead == 0) return; // Stream was closed
        Console.WriteLine(Encoding.ASCII.GetString(buffer, 0, bytesRead));
    }
}

In the initialization part you would do:

await client.Initialize(ip, port);
// Start reading task
Task.Run(() => client.Read());

For using synchronous methods delete all Async occurences and replace Task with Thread.

Gainey

It's ok. NetworkStream.ReadAsync returns 0 when the stream got closed from the remote side. However depending on the application it might make more sense to throw an exception here instead of just returning. This was just an example. However it seems an await was missing before ReadAsync in my example. I fixed that. – Gainey

What do you mean by closed from the remote side? How does that compare to when the message being sent has completed/reached EOF. But there could still be more messages in future. One thing im struggling to work out at the moment is how to keep reading for incoming messages from the stream with a connection that is never closed. MS docs has little examples of persistent connections, they close it soon after reading. – Versatile

Answer 3

You are missing an important property:

TcpClient.ReceiveBufferSize= 4096

That sets the size of the receiver buffer.

If the message is bigger than the buffer AUTOMATICALLY splits the messages for you without any extra code logic!

From the example above, having a buffer size with 4096 and the message len to send is 4097, then 2 packets will come to the receiver, one with 4096 (full buffer) and the other with 1 byte length. At this point is up to you to put in a concurrent queue (I know its a single client, but the method is an overlapped one, so handled like multi-threaded i think) and then inspect later (to avoid programming inside the socket event which can produce undesirable errors...Latency, etc). Someone said here a lenght=0 of the received message is an async error 10054, and less than 0, other types of error. Keep in mind to avoid too much code on receive event... Grab the data. put in a queue and nothing more at that point.

Morton