Creating a TCP Listener and receiving data

Question

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("<root>{0}</root>", 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());
 tcpClient.Close();
 tcpListener.Stop();
 ReceivePortMessages();
 return;
 }
 Debug.Print(" >> exit");
 }
}

Send Message

private void SendReply(string reply)
{
 try
 {
 NetworkStream networkStream = _TcpClient.GetStream();
 string serverResponse = reply;
 Byte[] sendBytes = Encoding.ASCII.GetBytes(serverResponse);
 networkStream.Write(sendBytes, 0, sendBytes.Length);
 networkStream.Flush();
 Debug.Print(" >> " + serverResponse);
 }
 catch (ArgumentOutOfRangeException ex)
 {
 Debug.Print("SendReply: Remote client disconnected. " + ex.ToString());
 _TcpClient.Close();
 _TcpListener.Stop();
 ReceivePortMessages();
 return;
 }
}

• dfhwze: Would you allow multiple clients to call your server concurrently or not? It makes a huge difference in implementation of your server.

Answer 1

1. 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.

2. 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.

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

4. 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.

5. 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.

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

7. 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("<root>{0}</root>", 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.

• Volearix: Okay, I will give this a shot, but the _QuitProcessing part, could you expand on that a bit since it is obviously a bool that you aren't defining.
• ChrisWue: Well _QuitProcessing is a bool class member which you can set in some other method if you want to stop the processing loop. You did originally only post the processing method and none of the supporting code so I don't know how you are running it.
• Volearix: Okay, I have updated my answer with a likely equally, if not more so, broken Send Message method.
• mrid: My AcceptTcpClient() isn't getting called. Can you please tell my why that might be happening? Other software are able to read from external device