views:

60

answers:

3

I am reading from a NetworkStream that is in a while loop. The issue is I am seeing 100% CPU usage. Is there any way to stop this from happening?

Here is what I have so far:

    while (client != null && client.Connected)
            {

                NetworkStream stream = client.GetStream();
                data = null;

                try
                {
                    // Check if we are still connected.
                    if (client.Client.Poll(0, SelectMode.SelectRead))
                    {
                        byte[] checkConn = new byte[1];

                        if (client.Client.Receive(checkConn, SocketFlags.Peek) == 0)
                        {
                            throw new IOException();
                        }
                    }

                    if (stream.DataAvailable)
                    {
                        //Read the first command
                        WriteToConsole("Waiting for next command");
                        data = ReadStringFromClient(client, stream);
                        WriteToConsole("Received Command: " + data);
                    }
                }

... Code continues...

ReadStringFromClient code:

   private string ReadStringFromClient(TcpClient clientATF, NetworkStream currentStream)
    {
        int i;
        string builtString;
        byte[] stringFromClient = new byte[256];

        if (clientATF.Connected && currentStream.CanRead)
        {

            i = currentStream.Read(stringFromClient, 0, stringFromClient.Length);
            builtString = System.Text.Encoding.ASCII.GetString(stringFromClient, 0, i);

        }

        else
        {
            return "Connection Error";
        }

        return builtString;

    }
+5  A: 

Your code contains a lot of... noise. You Ain't Gonna Need It.

The reason for 100% CPU load is that you're spin-waiting for data to become available. You don't need to do that. Read will block until data is available. You also don't need to recreate the NetworkStream for each chunk of data to receive.

Your code can be much simplified if you'd use a StreamReader:

using (var reader = new StreamReader(new NetworkStream(socket))
{
    char[] buffer = new char[512];
    int received;
    while ((received = reader.Read(buffer, 0, buffer.Length)) > 0)
    {
        string s = new string(buffer, 0, received);
        Console.WriteLine(s);
    }
}

Read block until data becomes available. The code loops while the connection is alive. You can simplify the code even further if you use ReadLine instead of reading into a char buffer.

If you don't want to block your thread until data becomes available, have a look at asynchronous reading.

dtb
I cleaned up my code a bit. Thanks for the input.
Sean P
+2  A: 

This is because you are constantly asking the CPU to do something for you - namely, to check if there is anything available on the stream.

This is a pretty common pattern that you need to learn to deal with appropriately. It is called asynchronous IO and the good news is that the .NET framework has extensive support to make it easy for you to accomplish what you want without running the CPU to 100% constantly.

The big problem is that your code is running in an endless loop, constantly calling client.Client.Poll(0, SelectMode.SelectRead) which will return immediately with an answer. Instead, you should ask the framework to notify you when something interesting happens, such as when there's data available to read from the network stream. And there's more than a handful of ways you can do that. One way is to use the BeginRead method of the NetowrkStream.

Here's an example of asynchronous sockets programming example for a client application.

Miky Dinescu
Ive started to look into this. Thanks
Sean P
+1  A: 

The reason you are seeing 100% cpu usage, is that you're always doing something, which is a result of you're endless while loop with no delays.

The basic semantics are:

  1. Check if client is connected
  2. Poll the client
  3. Check if data is available
  4. Read the data
  5. Go back to step 1

Since you're in this loop, you're always doing something, whatever it is. The easiest semantic is to do a Thread.sleep() at the end of you're loop. This will help you without having to make many changes. However, you will introduce a delay of whatever that sleep time is, and isn't really a proper way (but may suit you're situation).

The proper method is to learn about asynchronous sockets if you want a high performance server, or something that uses suitable low cpu when 1 or more sockets are connected. Doing a quick Google search is probably best to learn, I don't recall any particularly good articles off hand. The benefit of Asynchronous IO is basically that you will only use cpu time when you have something to do. When data is received, you're method will be called to do whatever processing you have to do.

Kevin Nisbet
This was the easiest solution however... the I now know Asynchronous sockets should have been my starting point.
Sean P