Implementing a good TCP Socket Server

Question

I'd like a code review on my very simple server application that validates whether the serial number retrieved from the client is a valid one or not.

1. Is there a better way to handle the start/stop on the service? If you look at my Start and Service methods, I'm basically looking at isServerRunning boolean variable and getting the service out of the while loop whenever it has been toggled to false, but this doesn't seem like the ideal approach since the thread is basically paused at listner.AcceptSocket().

2. Does this guarantee N:1 connectivity, 1 being the server?

3. I'm going to set a send/receive timeout on my socket. What is a generally reasonable timeout value to set?

I'd also like to hear comments on the overall coding style and any other suggestions on improving this code.

public partial class ServerForm : Form
{
 #region Fields
 private bool isServerRunning = false;
 private const int CLIENT_LIMIT = 10;
 private TcpListener listener;
 #endregion
 #region Event Handlers
 private void btnStart_Click(object sender, EventArgs e)
 {
 try
 {
 int port;
 if (String.IsNullOrEmpty(txtPort.Text))
 {
 MessageBox.Show(Constant.ERROR_PORT_NUMBER_EMPTY);
 return;
 }
 if (isServerRunning)
 return;
 if (!int.TryParse(txtPort.Text, out port))
 {
 MessageBox.Show(Constant.ERROR_PORT_ONLY_INT_ALLOWED);
 return;
 }
 if (port < 0 || port > 65535)
 {
 MessageBox.Show(Constant.ERROR_PORT_NUMBER_OUT_OF_RANGE);
 return;
 }
 Start(port);
 }
 catch (Exception ex)
 {
 LogWriter.WriteExceptionLog(ex.ToString());
 }
 }
 private void btnStop_Click(object sender, EventArgs e)
 {
 if (isServerRunning)
 {
 isServerRunning = false;
 ServerLogWriter("Server Stopped");
 }
 }
 private void btnClear_Click(object sender, EventArgs e)
 {
 try
 {
 listBoxLog.Items.Clear();
 }
 catch (Exception ex)
 {
 LogWriter.WriteExceptionLog(ex.ToString());
 }
 }
 #endregion
 #region Constructor
 public ServerForm()
 {
 InitializeComponent();
 }
 #endregion
 #region Private Methods
 private void ServerLogWriter(string content)
 {
 string log = DateTime.Now + " : " + content;
 this.BeginInvoke(new Action(() => 
 {
 listBoxLog.Items.Add(log);
 }
 ));
 LogWriter.WriteLog(log, Constant.NETWORK_LOG_PATH);
 }
 private void Start(int port)
 {
 try
 {
 isServerRunning = true;
 listener = new TcpListener(IPAddress.Parse(LocalIPAddress()), port);
 listener.Start();
 for (int i = 0; i < CLIENT_LIMIT; i++)
 {
 Thread t = new Thread(new ThreadStart(Service));
 t.IsBackground = true;
 t.Start();
 }
 ServerLogWriter(String.Format("Server Start (Port Number: {0})", port));
 }
 catch (SocketException ex)
 {
 ServerLogWriter(String.Format("Server Start Failure (Port Number: {0}) : {1}",port,ex.Message));
 LogWriter.WriteExceptionLog(ex.ToString());
 isServerRunning = false;
 }
 catch (Exception ex)
 {
 LogWriter.WriteExceptionLog(ex.ToString());
 isServerRunning = true;
 }
 }
 private void Service()
 {
 try
 {
 while (isServerRunning)
 {
 Socket soc = listener.AcceptSocket();
 ServerLogWriter(String.Format("Client Connected : {0}", soc.RemoteEndPoint));
 try
 {
 Stream s = new NetworkStream(soc);
 StreamReader sr = new StreamReader(s);
 StreamWriter sw = new StreamWriter(s);
 sw.AutoFlush = true;
 string validation = String.Empty;
 string serial = sr.ReadLine();
 if (ValidateSerial(serial))
 {
 validation = String.Format("Serial Number Validated (Received Serial: {1}) : {0}", soc.RemoteEndPoint, serial);
 sw.WriteLine("VALIDATE");
 }
 else
 {
 validation = String.Format("Invalid Serial Number (Received Serial: {1}) : {0}", soc.RemoteEndPoint, serial);
 sw.WriteLine("FAILURE");
 }
 ServerLogWriter(validation);
 s.Close();
 }
 catch (Exception ex)
 {
 ServerLogWriter(String.Format("Socket Error: {0}, {1}", soc.RemoteEndPoint, ex.Message));
 LogWriter.WriteLog(ex.ToString(), Constant.EXCEPTION_LOG_PATH);
 }
 ServerLogWriter(String.Format("End Connection : {0}", soc.RemoteEndPoint));
 soc.Close();
 }
 }
 catch (Exception ex)
 {
 LogWriter.WriteExceptionLog(ex.ToString());
 }
 }
 private string LocalIPAddress()
 {
 string localIP = String.Empty;
 try
 {
 IPHostEntry host;
 host = Dns.GetHostEntry(Dns.GetHostName());
 foreach (IPAddress ip in host.AddressList)
 {
 if (ip.AddressFamily.ToString() == "InterNetwork")
 {
 localIP = ip.ToString();
 }
 }
 return localIP;
 }
 catch (Exception ex)
 {
 LogWriter.WriteExceptionLog(ex.ToString());
 }
 return localIP;
 }
 #endregion
}

• Viezevingertjes: For serial number validation, is it not more appropriate to use WCF? have you looked at is as an option?
• user27446: See the following working example... C# Socket Sample Program Very simple and good for beginners.

Answer 1

You may want to look some other guides/implementations about that:
CodeProject | C# SocketAsyncEventArgs High Performance Socket
Stack Overflow | How to write a scalable Tcp/Ip based server

About your code:

• You should seperate the UI and the server code, so you can be able to use it on a console application or -more likely- a windows service.
• TcpListener is a good class that does its job pretty well but if you want something more scalable and customizable, I think it's too high-level for you. For this you should deal with Socket class directly.

You should also know,
There are so many concepts that you should be aware of about TCP and socket programming in general; in order to write a robust, scalable TCP server. You should know about framing protocols, find a good way to handle your buffers, be experienced at asynchronous code and debugging that code.

Edit: This FAQ page by Stephen Cleary is a really good place to get started.

I suggest you to take a look at the links I mentioned above. They provide some good implementations and explain why they do the things they do in a comprehensive way. Then you can roll your own, avoiding common pitfalls.

Back to your code:

• Creating a new thread for each connection is brutal. You should use the asynchronous methods of the TcpListener class. Or you can at least use ThreadPool and the easiest way to do this is using the Task class.
• LocalIPAddress() method returns a string that you parse again to an IPAddress object to use. Don't convert it to a string, return it directly as an IPAddress.
• AddressFamily is an Enum. You can check whether it is InterNetwork without converting it to a string: ip.AddressFamily == AddressFamily.InterNetwork
• Try to use your streams in using blocks or dispose them in finally blocks so you can be sure they're closed even if an exception is thrown in your code.
• Timeout really depends on the client, network and the size of the data you're sending/receiving. Whatever amount of inactive time you think that means "there's something wrong" will be a sufficient timeout value.

Nonetheless:
My advice is not to continue this and start another after you look into some other implementations like the ones I mentioned. You need a UI independent project that can be referenced by your other applications that need to use (or that need to be) a TCP server.

Edit: Although I said that you should be able to, it doesn't mean you should have a TCP server in a GUI application. You need a Windows service to run on the background, deal with high amounts of connections and manage the data traffic between itself and its clients. Doing these kinds of operations in a GUI application is usually a bad idea.

• TtT23: Ok. Thanks for the good answer. The reason why I was mixing GUI code with server code is that one of the requirements imposed on me was to show a log of connection status on the listbox. Any good ideas on how I should handle this?
• Şafak Gür: @l46kok: Is it have to be a ListBox? You can write your logs to a database or a log file and then you can create a GUI application (say, LogViewer) to query and show the logs. You shouldn't keep all the logs in the memory anyway since it will eventually cause your program to throw an OutOfmemoryException and there is the fact that all your logs would be gone when you close the application. For LogViewer, you should get the last N logs depending on your environment. (I don't think a ListBox with millions of items would perform well)
• TtT23: @ŞafakGür As much as how weird it sounds, yes it has to be a ListBox because that is the requirement imposed on me. The ListBox is automatically cleared out when it reaches a certain number of items, so memory isn't that huge of a problem.
• Şafak Gür: @l46kok: Then you can use it in your log viewer. You should be able to see the yesterday's or last month's logs if you want to. So saving the logs somewhere more persistent (like DB or file system) is the way to go. You can use whatever control you wish to show the logs in your log viewer. Think this way: Your TCP server will be running all the time but not everyone will look at its logs every second, constantly. So creating a Windows service for the TCP Server and creating a GUI application that people will run to review logs and close when they're done seems like the right thing to do here.

Answer 2

Without reviewing other aspects, I think you should separate the code so that there's the server itself in its class (or, maybe even better, its own project), and there's the UI.

For instance, you should not show a MessageBox inside a server's code. There are so many reasons for that so I'll just leave this statement as is.

In other words, the server should behave as a black box, returning error codes, or throwing exceptions, or anything else, according to its own API. Mixing the server's code with UI is not a good practice.