Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
}
share|improve this question
    
For serial number validation, is it not more appropriate to use WCF? have you looked at is as an option? –  MrMichael Nov 2 '12 at 10:24
    
See the following working example... C# Socket Sample Program Very simple and good for beginners. –  user27446 Jul 19 '13 at 8:03

2 Answers 2

up vote 11 down vote accepted

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.

share|improve this answer
    
One thing you alluded to, but didn't say explicitly: having server with a GUI is a really bad idea. Server should run as a Windows service, or something like that. Although having a GUI application for testing is probably okay. –  svick Oct 23 '12 at 19:13
    
@svick: I edited my answer to state that explicitly at the end, thanks. –  Şafak Gür Oct 23 '12 at 21:01
    
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? –  l46kok Oct 24 '12 at 0:09
    
@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) –  Şafak Gür Oct 24 '12 at 6:00
    
@Ş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. –  l46kok Oct 24 '12 at 6:16

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.

share|improve this answer

protected by Jamal Feb 5 at 6:29

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.