Transmitting/Sending big packet safely using TcpClient/Socket

Question

I am writing a TCP based client that need to send and receive data. I have used the Asynchronous Programming Model (APM) provided for Socket class by the .NET Framework.

After being connected to the socket, I start waiting for data on the socket using BeginReceive.

Now, while I am waiting for the data on the Socket, I may need to send data over the socket. And the send method can be called multiple times,

So I have make sure that

• All the bytes from previous Send call is entirely sent.
• The way I am sending the Data is safe considering that, while a data send is in progress, any call to send data can be made.

This is my first work on socket, So is my approach right to send data ?

 private readonly object writeLock = new object();
 public void Send(NetworkCommand cmd)
 {
 var data = cmd.ToBytesWithLengthPrefix();
 ThreadPool.QueueUserWorkItem(AsyncDataSent, data);
 }
 private int bytesSent;
 private void AsyncDataSent(object odata)
 {
 lock (writeLock)
 {
 var data = (byte[])odata;
 int total = data.Length;
 bytesSent = 0;
 int buf = Globals.BUFFER_SIZE;
 while (bytesSent < total)
 {
 if (total - bytesSent < Globals.BUFFER_SIZE)
 {
 buf = total - bytesSent;
 }
 IAsyncResult ar = socket.BeginSend(data, bytesSent, buf, SocketFlags.None, DataSentCallback, data);
 ar.AsyncWaitHandle.WaitOne();
 }
 }
 }

How object is changed into byte[], sometimes the NetworkCommand can be as big as 0.5 MB

 public byte[] ToBytesWithLengthPrefix()
 {
 var stream = new MemoryStream();
 try
 {
 Serializer.SerializeWithLengthPrefix(stream, this, PrefixStyle.Fixed32);
 return stream.ToArray();
 }
 finally
 {
 stream.Close();
 stream.Dispose();
 }
 }

• svick: Why are you using the asynchronous method BeginSend() when you immediately synchronously wait for it to complete? Why don't you just use Send()? Also, can you use C# 5.0?
• Parimal Raj: no i am limited to .net 3.5, i have to use BeginSend as i am already awaiting for data to be received : stackoverflow.com/questions/17356507/…

Answer 1

Instead of using the ToArray() method of the MemoryStream you could use GetBuffer() instead because this isn't creating a new array.

Edit based on @Iridium's comment

But this is only good if you use one of the constructor that creates a not resizable MemoryStream which isn't the case for the default constructor.

Calling Dispose() on the MemoryStream closes the stream too.

If you enclose the stream inside inside a using block the disposing of the stream will be handled automatically.

public byte[] ToBytesWithLengthPrefix()
{
 using (var stream = new MemoryStream())
 {
 Serializer.SerializeWithLengthPrefix(stream, this, PrefixStyle.Fixed32);
 return stream.ToArray();
 }
}

---

I needed to look twice at var data = (byte[])odata; which is a clear sign that you should make the method argument odata easier to distinguish from data, so objectData or dataToSend would be better.

int buf isn't well named either. One shouldn't use abbreviations to name variables. A much better name would be for instance dataSize or just size to indicate that it is the amount of data being sent.

• Iridium: The internal buffer returned by GetBuffer() may be larger than the stream's Length. Since the true length is not also returned, this could result in sending more data than expected (and more than indicated by the prefixed length indicator).