5

I am writing an app that will require to make hundreds of socket connections over tcp to read/write data.

I have come across this code snippet here and I'm wondering how I can make this more robust.

This is currently how I am calling the code:

foreach (var ip in listofIps)
{
   IPEndPoint remoteEP = new IPEndPoint(IPAddress.Parse(ip), 4001);
   Socket client = new Socket(AddressFamily.InterNetwork,
                           SocketType.Stream, ProtocolType.Tcp);
   client.Connect(remoteEP);
   await ReadAsync(client);
}
  1. Is there anything wrong with the above, and how can it be optimized such that it runs concurrently?

    In the code snippet, the buffer size is set to 1000. Just as a simple illustration, if I were to attempt to print out only the bytes received, and not the remaining 0x00s, I have to do something like this:

    while (true)
    {
        await s.ReceiveAsync(awaitable);
        int bytesRead = args.BytesTransferred;
        if (bytesRead <= 0) break;
        var hex = new StringBuilder(bytesRead * 2);
        var msg = new byte[bytesRead];
    
        for (int i = 0; i < bytesRead; i++)                
            msg[i] = args.Buffer[i];                
    
        foreach (byte b in msg)                
            hex.AppendFormat("{0:x2} ", b);
    
        AppendLog(string.Format("RX: {0}", hex));
    }
    
  2. Is there a more efficient way of doing this? Previously, I would iterate the whole buffer and print out the data, but that will give me a whole bunch of trailing 0x00s as my protocol is anywhere between 60 to 70 bytes long.

Null Reference
  • 11,260
  • 40
  • 107
  • 184
  • 2
    You may not get many replies to this question as it is overly broad and you ask such as 'can it be further optimized?'. The answers is generally a resounding yes. – MarcF Jun 13 '13 at 17:28
  • I did state the areas I wanted it to be optimized. I don't think the question is as broad as you make it sound. I didn't ask "can this be optimized?" in general – Null Reference Jun 13 '13 at 17:32
  • @PapyrusBit But you asked three or four questions that are not directly related. You should ask only one question at a time. – svick Jun 13 '13 at 17:47
  • 1
    That code is currently awaiting each *in turn* - rather than concurrently. Is that really your intention? – Marc Gravell Jun 13 '13 at 17:55
  • @MarcGravell Thank you for pointing that out. I want them to run concurrently, how should the code be? – Null Reference Jun 13 '13 at 18:01
  • @svick Ok, I have trimmed down my question, and I hope this is more precise – Null Reference Jun 13 '13 at 18:03

1 Answers1

18

I am writing an app that will require to make hundreds of socket connections over tcp to read/write data.

You don't need "high-performance sockets" for that. The code is far simpler with regular-performance sockets.

For starters, don't use the custom awaitables from the link you posted. They are perfectly fine for some people (and completely "robust"), but you don't need them and your code will be simpler without them.

  1. Is there anything wrong with the above, and can it be further optimized?

Yes. You shouldn't mix blocking (Connect) and asynchronous (ReadAsync) code. I would recommend something like this:

foreach (var ip in listofIps)
{
  IPEndPoint remoteEP = new IPEndPoint(IPAddress.Parse(ip), 4001);
  Socket client = new Socket(AddressFamily.InterNetwork,
                             SocketType.Stream, ProtocolType.Tcp);
  await client.ConnectTaskAsync(remoteEP);
  ...
}

Where ConnectTaskAsync is a standard TAP-over-APM wrapper:

public static Task ConnectTaskAsync(this Socket socket, EndPoint endpoint)
{
  return TaskFactory.FromAsync(socket.BeginConnect, socket.EndConnect, endpoint, null);
}

As Marc Gravell pointed out, this code (and your original code) is connecting the sockets one at a time. You could use Task.WhenAll to connect them all simultaneously.

2) Is there a more efficient way of doing this?

First, you should define a TAP-over-APM ReceiveTaskAsync wrapper similar to the above. When dealing with binary data, I also like to have an extension method on byte arrays for dumping:

public string DumpHex(this ArraySegment<byte> data)
{
  return string.Join(" ", data.Select(b => b.ToString("X2")));
}

Then you can have code like this:

while (true)
{
  int bytesRead = await socket.ReceiveTaskAsync(buffer);
  if (bytesRead == 0) break;
  var data = new ArraySegment<byte>(buffer, 0, bytesRead);
  AppendLog("RX: " + data.HexDump());
  ...
}

If you do a lot of binary manipulation, you may find my ArraySegments library helpful.

3) Where and how should I include the logic to check if my whole data has arrived within a single read

Oh, it's more complex than that. :) Sockets are a stream abstraction, not a message abstraction. So if you want to define "messages" in your protocol, you need to include a length prefix or delimiter byte so you can detect the message boundaries. Then you need to write code that will parse out your messages, keeping in mind that blocks of data read from the socket may contain only a partial message (so you have to buffer it), a complete message, multiple complete messages, and may also end with a partial message (again, buffering). And you have to also consider your existing buffer when receiving the new block.

I have a TCP/IP .NET Sockets FAQ on my blog that addresses this specifically and has some example code using my personal default preference for message framing (4-byte little-endian length prefixing).

4) How should I include a writeasync method such that I can send data through the socket in the middle of reads.

That one's surprisingly tricky:

public static Task<int> SendTaskAsync(this Socket socket, byte[] buffer, int offset, int size, SocketFlags flags)
{
  return Task<int>.Factory.FromAsync(socket.BeginSend, socket.EndSend, buffer, offset, size, flags, null);
}
public static Task WriteAsync(this Socket socket, byte[] buffer)
{
  int bytesSent = 0;
  while (bytesSent != buffer.Length)
  {
    bytesSent += await socket.SendTaskAsync(data, bytesSent, buffer.Length - bytesSent, SocketFlags.None);
  }
}
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • In your last piece of code: 1. `SendTaskAsync()` should return `Task`, not just `int`. 2. Why are you boxing `0` as `state`? I think `null` makes more sense. 3. Wouldn't it make more sense to use the `byte[], int, int` overload instead of the `IList>` overload? – svick Jun 13 '13 at 18:31
  • @svick: Thanks for the fixes. Updated. "0" is a unfortunate habit I have since I did a lot of embedded programming in C over the last 18 months and there is no "null" in C. I used the `ArraySegment` overload so I wouldn't have to copy arrays around if the write only partially went through (rare, but possible). – Stephen Cleary Jun 13 '13 at 18:34
  • With the other overload, you still wouldn't have to copy anything, you would just call the method again with different `offset`. And you wouldn't have to create the single-element array. – svick Jun 13 '13 at 18:39
  • @RoyiNamir: It's defined in my answer. – Stephen Cleary Mar 26 '14 at 12:19
  • @StephenCleary that's what is happening When I try to follow line by line in linqpad and dont read further :P – Royi Namir Mar 26 '14 at 12:20
  • @StephenCleary in the extension method : this is what worked for me `return Task.Factory.FromAsync` (after _Cannot access non-static method FromAsync" in static context_) . but still im getting error : _cannot convert from 'System.Net.IPEndPoint' to 'System.Net.Sockets.SocketAsyncEventArgs'_ - in `.ConnectAsync( here` – Royi Namir Mar 26 '14 at 12:25
  • @RoyiNamir: Nowhere in my code do I call `ConnectAsync`. – Stephen Cleary Mar 26 '14 at 12:28
  • @StephenCleary Ignore the last stupid comment. In the last section : the code for `public static Task SendTaskAsync(t` is not compiling . at first it says : _cannot convert from '' to 'System.Threading.Tasks.TaskCreationOptions'_ and then after setting to "None" it says : _cannot convert from 'method group' to 'System.Func'_ ( sorry for commenting , just want to create working sample) – Royi Namir Mar 26 '14 at 12:44
  • @RoyiNamir: Perhaps you should ask your own question; I'm having a hard time following it in the comments. – Stephen Cleary Mar 26 '14 at 12:59
  • @StephenCleary I asked [here](http://stackoverflow.com/questions/22662621/server-communication-via-async-await-via-tap) , refering to the common Extension methods , but it seems that the task is never signaled... I would love if (in your spare time) to have a look. – Royi Namir Mar 26 '14 at 13:44