Creating a TCP Listener and receiving data

I am very new to network programming and I'm thinking I have probably misconstrued the creation of an appropriate TCP listener. The code I have below works perfectly, but I have a feeling it's a "hack and slash" miracle and could come apart easily when set into my production environment. What can I do to improve this receive message?

Objective

The application should be able to receive a string and parse it into an xml string. Later, after a set of validation and data collection is completed, a response will be sent back to the client.

Receive Message

```
private static void ReceivePortMessages()
{
int requestCount = 0;
tcpListener.Start();
Debug.Print(" >> Server Started");
tcpClient = tcpListener.AcceptTcpClient();
Debug.Print(" >> Accept connection from client");

while (true)
{
try
{
requestCount = requestCount++;
NetworkStream networkStream = tcpClient.GetStream();
byte[] bytesFrom = new byte[10025];
networkStream.Read(bytesFrom, 0, (int)tcpClient.ReceiveBufferSize);
Stopwatch sw = new Stopwatch();
sw.Start();
string dataFromClient = System.Text.Encoding.ASCII.GetString(bytesFrom);
dataFromClient = dataFromClient.Substring(0, dataFromClient.IndexOf("\0"));

XmlDocument xm = new XmlDocument();
xm.LoadXml(string.Format("{0}", dataFromClient));
XmlElement root = xm.DocumentElement;
string rootName = root.FirstChild.Name;
RouteInboundXML(rootName, dataFromClient, sw);
}
catch (ArgumentOutOfRangeException ex)
{
Debug.Print("ReceivePortMessages: Remote client disconnected. " + ex.ToString());
tcpClient.Close();
tcpListener.Stop();
ReceivePortMessages();
return;
}
catch (Exception ex)
{
Debug.Print("ReceivePortMessages: " + ex.ToString());

-
A common convention is to prefix class members with _ so it's easier to see what a local variable is and what's a class member. In your case it would be _tcpClient or even _TcpClient.

-
This is a static method which indicates that it's possibly part of a static class. This is almost always a bad idea. static means it's global and global state is bad because it can get you into all kinds of trouble once your program grows in complexity.

-
You have a StopWatch but you're not actually using it.

-
I assume you use the Substring because the buffer is larger than the actual data received so you find the end by looking for the first \0. GetString() provides an overload which allows you to specify offset and length so this becomes unnecessary.

-
MSDN stipulates that you must close the NetworkStream once you are done with it because the client will not release it. Given that streams are IDisposable it would be best wrapped in a using block.

-
TcpClient is IDisposable as well so should be wrapped in a using block to make sure it gets cleaned up properly.

-
The method is recursive and there is no way to bail out because in case of error it calls itself again. This means your call stack is growing indefinitely and will fall over at some point. Each call will also allocate a new receive buffer every time.

A while loop with a break condition would probably be the better choice.

So in total the refactored code could look like this:

private static void ReceivePortMessages() 
{
    byte[] receiveBuffer = new byte[10025];
    while (!_QuitProcessing)
    {
        int requestCount = 0;
        _TcpListener.Start();
        Debug.Print(" >> Listener Started");
        using (var tcpClient = _TcpListener.AcceptTcpClient())
        {
            Debug.Print(" >> Accepted connection from client");

            using (var networkStream = tcpClient.GetStream())
            {
                while (!_QuitProcessing)
                {
                    try
                    {
                        requestCount = requestCount++;
                        var bytesRead = networkStream.Read(receiveBuffer, 0, (int)tcpClient.ReceiveBufferSize);
                        if (bytesRead == 0)
                        {
                            // Read returns 0 if the client closes the connection
                            break;
                        }

                        string dataFromClient = System.Text.Encoding.ASCII.GetString(receiveBuffer, 0, bytesRead);

                        XmlDocument xm = new XmlDocument();
                        xm.LoadXml(string.Format("{0}", dataFromClient));
                        XmlElement root = xm.DocumentElement;
                        string rootName = root.FirstChild.Name;
                        RouteInboundXML(rootName, dataFromClient, sw);
                    }
                    catch (Exception ex)
                    {
                        Debug.Print("ReceivePortMessages: " + ex.ToString());
                        break;
                    }
                }
            }
            Debug.Print(" >> stopped read loop");
        }
        _TcpListener.Stop();
    }
}

It's still not ideal because the Read will block unless data is there to be received or the connection is closed. So if the other side stops sending data but keeps the connection alive you still have no nice way to bail out.

StackExchange Code Review Q#78325, answer score: 4