TcpClient BottleNeck

Question

I'm trying to create a self-contained class which maintains a Tcp connection to a server.

I'm using the following class variables:

TcpClient tcpClient;
NetworkStream networkStream;
BinaryReader mReader;
BinaryWriter mWriter;

And initializing them using the following code:

tcpClient = new TcpClient(host, 443);
networkStream = tcpClient.GetStream();
mReader = new BinaryReader(networkStream);
mWriter = new BinaryWriter(networkStream);

receiveMessage = new Thread(new ThreadStart(ReceiveMessages));
receiveMessage.Start();

I'm using blocking calls for the reading. Each packet coming from the server is prefixed with 4 bytes (an int) which define the exact packet size. I'm using a class I wrote named ByteBuffer which has a List(Byte) to store the bytes once they come in. The class has functions which pull ints and other types off of the top of the byte list using ReadInt(), ReadString() etc, according to the server protocol.

Here is the Receiver thread:

private void ReceiveMessages()
{
    while (tcpClient.Connected)
    {
        if (tcpClient.Available >= 4)
        {
            try
            {
                ByteBuffer message = new ByteBuffer();
                message.AddBytes(mReader.ReadBytes(4));
                int mSize = message.ReadInt();
                message.AddBytes(mReader.ReadBytes(mSize - 4));
                MessageProcessor.Process(message);
            }
            catch (Exception ex)
            {
               Print(ex.Message);
            }
        }
        Thread.Sleep(1);
    }
    Print("Receiver thread terminated.");
    Reconnect();
}

For reference, the MessageProcessor is a static class which looks at the packet information and responds to the server appropriately.

My problem is that when the traffic on the connection begins to get really high, the responses begin to get significantly delayed. I'm wondering, is there something I'm doing incorrectly as far as the tcp connection is concerned? Should I try writing an asynchronous version of the class instead? Is the C# List object too slow to be used this frequently (in the ByteBuffer)?

This is really my first attempt at network programming, so any advice would be extremely helpful.

Thanks.

Wait... can't you just read an integer straight from a BinaryReader mReader? Why do you need a ByteBuffer message? BinaryReader doc: http://msdn.microsoft.com/en-us/library/system.io.binaryreader.aspx — John McDonald, Aug 17 '11 at 19:37
I believe the server uses big-endian, and the BinaryReader uses little-endian. Or other way around. I can't remember which. — jjw, Aug 17 '11 at 19:54
Ok. If that's the case, then I don't see anything really wrong with what you are doing. If you want to keep this single-threaded, I'd recommend using a Profiler to help figure out where the slow-down is. If your MessageProcessor.Process(message) takes some processing time, I'd recommend going multi-threaded. — John McDonald, Aug 17 '11 at 20:10
FYI, This post seems to show that List is essentially a fixed array: http://stackoverflow.com/questions/2540050/how-is-listt-implemented-in-c So using a List is just fine too — John McDonald, Aug 17 '11 at 20:24
Thanks for the thread, that makes me feel better about using the List. I have used a Profiler, and can't seem to find where the code is getting held up at. It spent the majority of it's time on the line "if (tcpClient.Available >= 4)", which I assumed was just the time between packets. The only reason I'm aware that it's slowing down is when I'm looking at the responses it's sending out (in the application which uses the server), I see them at 3-4 second delay from they should be. I suppose the Sending function could have some issues, but the sending is used less often (2 packets / 500ms) — jjw, Aug 17 '11 at 20:51
Yeah, I was expecting the majority of time on the .Available as well, that just means it's IO-bound, big surprise. For these outgoing packets, are you reusing existing connections or making new ones? TCP has some connection overhead, the overhead is largely based on the ping between the end points though. — John McDonald, Aug 17 '11 at 20:59
I'm re-using the binaryWriter that was shown above when everything was created. The Send function just takes an array of bytes, and does: mWriter.Write(send); mWriter.Flush(); — jjw, Aug 17 '11 at 21:06

Yavor Shahpasov · Answer 1 · 2011-08-17T20:39:41.340

2

I would rewrite your ReceiveMessages method like so Removes the Thread.Sleep which is bad. Uses byte arrays which are faster.

Like @jgauffin said Async network code is much better but it is easier to mess up. If you are just starting with network programming better keep it simple.

I hope this works better for you.

Note message is without the 4 byte header

private void ReceiveMessages()
    {

        while (tcpClient.Connected) {
            try {

                var networkstream = tcpClient.GetStream();
                var header = new byte[4];
                networkstream.Read(header, 0, 4);

                int len = 0;
                // calculate length from header
                // Do reverse for BigEndian, for little endian remove
                Array.Reverse(header);
                len = BitConverter.ToInt32(header, 0);

                var message = new byte[len];
                networkstream.Read(message, 0, message.Length);

                // Process message

            }
            catch (Exception ex)
            {
                Print(ex.Message);
                // Exit loop something went wrong
                break;
            }
        }

        Print("Receiver thread terminated.");
        Reconnect();

    }

edited Aug 17 '11 at 20:39

answered Aug 17 '11 at 20:19

Yavor Shahpasov

1,453
1
12
19

Thanks for that code, I'll give it a try as soon as possible. The Thread.Sleep() was in there because without it, the program was using a significant portion of the CPU. Is there any other way to avoid this? – jjw Aug 17 '11 at 20:37
In this code networkstream.Read will block until enough bytes are available so no need for Thread.Sleep – Yavor Shahpasov Aug 17 '11 at 20:40
Oh, I'd missed that you'd removed checking the bytes available. This makes much more sense than what I'm doing. Thanks – jjw Aug 17 '11 at 20:53
I tried removing the lines to check if 4 bytes are available, and removed the sleep, and I'm getting the following exception: "Unable to read data from the transport connection: A blocking operation was interrupted by a call to WSACancelBlockingCall." Any idea what I did wrong? – jjw Aug 18 '11 at 20:18
I would need to see the full program to answer. I would guess that this is happening because you are interacting with the socket outside of the thread or due to external factors (link dropped from the server). A more complete code would take care of synchronization on the TcpSocket (lock - http://msdn.microsoft.com/en-us/library/c5kehkcz(v=vs.71).aspx is a simple way to do that) or if the TcpClient is only needed in the thread you should put the initialization of TcpSocket inside ReceiveMessages. If you are using the same socket to send messages best that happens in the same thread. – Yavor Shahpasov Aug 18 '11 at 20:37

jgauffin · Answer 2 · 2012-05-05T10:42:55.677

0

Switch to asynchronous handling instead. Using a thread and Thread.Sleep is evil.

Look at Socket.BeginReceive or in your case NetworkStream.BeginRead

You could also use my new library: http://blog.gauffin.org/2012/05/griffin-networking-a-somewhat-performant-networking-library-for-net/

edited May 05 '12 at 10:42

answered Aug 17 '11 at 19:40

jgauffin

99,844
45
235
372

TcpClient BottleNeck

2 Answers2

Linked