3
\$\begingroup\$

I am trying to write a simple TCP server in C#. It needs to be able to receive packets and do work on them. Ideally it will only be receiving packets from a single computer, but I'm making it able to handle multiple packets to account for corporate network scans. The server will not need to manage multiple long-running connections, so I was trying to keep it as simple as possible (the async examples all looked complex with many functions to handle connections, data reads, and disconnects). All packets will be signed and verified by an RSA key, which should take care of the network scans and partial receives. What else can be done to improve the robustness of the following program?

using System;

namespace Base_TCP_Server
{
    class Program
    {
        static int Port = 7110;//code from Bomberman
        static int MaxThreads = 2;//MAX number of threads, 2 for testing > threads connections
        static int DataReadTimeoutMS = 2000;
        static System.Threading.WaitHandle[] WaitHandles;
        static System.Net.Sockets.Socket ListenSocket;

        struct ThreadParamsStruct
        {
            public System.Threading.AutoResetEvent ThreadHandle;
            public System.Net.Sockets.Socket ClientSocket;
            public int ThreadIndex;
        }

        static void Main(string[] args)
        {
            WaitHandles = new System.Threading.WaitHandle[MaxThreads];
            for (int i = 0; i < MaxThreads; ++i)
                WaitHandles[i] = new System.Threading.AutoResetEvent(true);
            ListenSocket = new System.Net.Sockets.Socket(System.Net.Sockets.SocketType.Stream, System.Net.Sockets.ProtocolType.Tcp);
            ListenSocket.Bind(new System.Net.IPEndPoint(0, Port));
            ListenSocket.Listen(25);
            for (;;)
            {//accept connections and send socket to other thread
                Console.WriteLine("Waiting for a connection");
                System.Net.Sockets.Socket sock = ListenSocket.Accept();
                Console.WriteLine("Got a connection");
                Console.WriteLine("Waiting for idle thread");
                int index = System.Threading.WaitHandle.WaitAny(WaitHandles);
                Console.WriteLine("Starting new thread to process client");
                ThreadParamsStruct p = new ThreadParamsStruct() { ThreadHandle = (System.Threading.AutoResetEvent)WaitHandles[index], ClientSocket = sock, ThreadIndex = index };
                System.Threading.ThreadPool.QueueUserWorkItem(new System.Threading.WaitCallback(ProcessSocketConnection), p);
            }
        }

        static void ProcessSocketConnection(Object threadState)
        {
            ThreadParamsStruct ts = (ThreadParamsStruct)threadState;
            Console.WriteLine(string.Format("Thread {0} is processing connection {1}", ts.ThreadIndex, ts.ClientSocket.RemoteEndPoint.ToString()));
            byte[] fakeStorage = new byte[1024 * 256];
            if (ts.ClientSocket.Poll(DataReadTimeoutMS * 1000, System.Net.Sockets.SelectMode.SelectRead))
            {
                ts.ClientSocket.Receive(fakeStorage);
            }
            else
            {
                Console.WriteLine("Got no data, aborting");
                goto cleanup;
            }

            //do data processing/verification/etc. here
            if (!true)//verification goes here
            {//may need this for a send that didn't make it all the way??? required for signature verification at least
                Console.WriteLine("Data failed verification");
                goto cleanup;
            }
            Console.WriteLine("Doing 5 seconds worth of work on data");
            DateTime current = DateTime.Now;
            current = current.AddSeconds(5);
            while (DateTime.Now < current)
                ;

            cleanup:
            Console.WriteLine("Doing clean up tasks");
            ts.ClientSocket.Shutdown(System.Net.Sockets.SocketShutdown.Both);
            ts.ClientSocket.Close();
            ts.ClientSocket.Dispose();//not sure if dispose calls the above 2 lines
            for (int i = 0; i < fakeStorage.Length; ++i)
                fakeStorage[i] = 0;
            ts.ThreadHandle.Set();
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

I am kinda new to reviewing code but I know c# kind good and will try my best.

First, let's go over some code style issues. At the top you declared variables like this:

    private static int Port = 7110;//code from Bomberman
    private static int MaxThreads = 2; //MAX number of threads, 2 for testing > threads connections
    private static int DataReadTimeoutMS = 2000;

Values that are unlikely to change, like port numbers, should be constants or read-only fields to prevent modification. So this should be:

private const int PORT = 7110;
private const int MAX_THREAD_NUMBER = 2;
private const int DATA_READ_TIMEOUT = 2000;

Also, private members of a class should be named in lowerCamelCase. But since these are now constants you should use all Uppercase casing which should be separated by underscores.

As a side note, the comments are also kind of unnecessary since the variable name should already be enough of an indication of what the code is doing. Code should explain itself, comments should explain the "Why"

Next, let's go over the naming of variables. In the above example, you named a variable "DataReadTimeoutMS" notice the "MS" at the end should indicate that this time is in Milliseconds which may be a good idea but this is a form of hungarian notation. You should avoid naming things like this. Timeout is already indicating a time.

You set up some more Variables further down like this:

private static System.Threading.WaitHandle[] WaitHandles;
private static System.Net.Sockets.Socket ListenSocket;

You see you are directly accessing the namespace of the Object you want to use, this creates a lot of code smell and makes it hard to read. Better would be importing the namespace. This enables you to shorten all the "System.Net.Sockets.XYZ" to "XYZ" alone

using System.Net;
using System.Net.Sockets;
using System.Threading;

Next, you define a struct which I find is a good use for encapsulating some data. Except for the Name, the name should not contain "Struct" ( Again Hungarian Notation )

You then use a for loop to assign the indexes of the WaitHandles, you should always use parenthesis when using c# this is commonly accepted and even though using whitespace is valid it can introduce bugs more easily so better would be:

for (int i = 0; i < MAX_THREADS; i++)
{
    waitHandles[i] = new AutoResetEvent(true);
}

Then you assign the "listenSocket" which really should be named "listenerSocket" or just "listener" and bind it to a new IPEndPoint and start listening.

When you are using a loop that will run for a long time or until a condition is false using a while loop is more appropriate. Instead of a for(;;) loop.

In the loop, you are starting to accept with the socket and write out to the console when a client made a connection. All of this code is very much condensed and hard to read try to use new lines to separate your code a bit more.

Then you wrote:

    ThreadParamsStruct p = new ThreadParamsStruct() { ThreadHandle = (System.Threading.AutoResetEvent)waitHandles[index], ClientSocket = sock, ThreadIndex = index };

Generally, "p" is not a very good variable name it says very little about what value it should hold. I personally would also prefer having a constructor to call instead of using an object initializer, but this is personal preference however when using an object initializer you should put it on a new line. Also, I would use the "var" keyword since the type is already visible and does not have to be mentioned twice.

When assigning a delegate or using one to set up a handler you don't need to add the Type you can simply use the Method as a parameter.

ThreadPool.QueueUserWorkItem(new System.Threading.WaitCallback(ProcessSocketConnection), p);

So this should be:

ThreadPool.QueueUserWorkItem(ProcessSocketConnection, p);

The callback given in the parameter is "ProcessSocketConnection" it takes an "Object" as a parameter you should use the builtin c# alias "object".

Since c# 6 you can use String interpolation. This helps with readability. So you should convert:

Console.WriteLine(string.Format("Thread {0} is processing connection {1}", state.ThreadIndex, state.ClientSocket.RemoteEndPoint.ToString()));

TO

Console.WriteLine($"Thread {state.ThreadIndex} is processing connection{state.ClientSocket.RemoteEndPoint}");

Also, notice you don't need to call ".ToString()" this will automatically be done.

You should not use magic numbers like "1024 * 256" in the code you should try to convert them to Constants. Also, you are multiplying your "DataReadTimeoutMS" by 1000 as a reader I must ask myself "why 1000" and since this is the only time this constant is used why not make the constant that value. Like this:

private const int DATA_READ_TIMEOUT = 2_000_000;

Since c# 7 you can also use number separators between numbers to make them more readable.

In the following, if block you use a "goto" goto's are generally considered a very bad practice since they make the code hard to follow and nonmodular. The cleanup should be a Method or a local Function like.

            void Cleanup()
            {
               Console.WriteLine("Doing clean up tasks");
               state.ClientSocket.Shutdown(SocketShutdown.Both);
               state.ClientSocket.Close();
               state.ClientSocket.Dispose();

               recievBuffer = new byte[STORAGE_SIZE];

               state.ThreadHandle.Set();
            }

In the Cleanup code instead of going over all indexes of the buffer you can simply assign the variable a new array.

recievBuffer = new byte[STORAGE_SIZE];

// https://xkcd.com/292/

I will ignore the part where you will want to put your data processing. But as a sidenote comments should never be on the same line as the code is.

Also, in the following section you do this:

DateTime current = DateTime.Now;
current = current.AddSeconds(5);

you can shorten this to:

DateTime futureTime = DateTime.Now.AddSeconds(5);
while (DateTime.Now < futureTime)
{ }

Here is my refactored code:

namespace Base_TCP_Server
{

using System;
using System.Net;
using System.Net.Sockets;
using System.Threading;

internal class Program
{
    private const int PORT = 7110;
    private const int MAX_THREADS = 2;
    private const int DATA_READ_TIMEOUT = 2_000_000;
    private const int STORAGE_SIZE = 1024 * 256;

    private static WaitHandle[] waitHandles;
    private static Socket listener;

    private struct ThreadParams
    {
        public AutoResetEvent ThreadHandle;
        public Socket ClientSocket;
        public int ThreadIndex;
    }

    private static void Main(string[] args)
    {
        waitHandles = new WaitHandle[MAX_THREADS];
        for (int i = 0; i < MAX_THREADS; ++i)
        {
            waitHandles[i] = new AutoResetEvent(true);
        }

        listener = new Socket(SocketType.Stream, ProtocolType.Tcp);
        listener.Bind(new IPEndPoint(IPAddress.Any, PORT));
        listener.Listen(25);

        while (true)
        {
            Console.WriteLine("Waiting for a connection");
            Socket sock = listener.Accept();

            Console.WriteLine("Got a connection");
            Console.WriteLine("Waiting for idle thread");
            int index = WaitHandle.WaitAny(waitHandles);

            Console.WriteLine("Starting new thread to process client");

            ThreadParams context = new ThreadParams() 
            {
               ThreadHandle = (AutoResetEvent)waitHandles[index], 
               ClientSocket = sock, ThreadIndex = index 
            };

            ThreadPool.QueueUserWorkItem(ProcessSocketConnection, context);
        }
    }

    private static void ProcessSocketConnection(object threadState)
    {
        ThreadParams state = (ThreadParams)threadState;
        Console.WriteLine($"Thread {state.ThreadIndex} is processing connection{state.ClientSocket.RemoteEndPoint}");

        // This should be an extra method. In general this code should be more modular!
        byte[] recievBuffer = new byte[STORAGE_SIZE];
        if (state.ClientSocket.Poll(DATA_READ_TIMEOUT, SelectMode.SelectRead))
        {
            state.ClientSocket.Receive(recievBuffer);
        }
        else
        {
            Console.WriteLine("Got no data, aborting");
            Cleanup();
        }

        // Do your data Processing in this Method.
        DoWork(recievBuffer);

        // This is a local Function introduced in c#7
        void Cleanup()
        {
            Console.WriteLine("Doing clean up tasks");
            state.ClientSocket.Shutdown(SocketShutdown.Both);
            state.ClientSocket.Close();
            state.ClientSocket.Dispose();

            recievBuffer = new byte[STORAGE_SIZE];

            state.ThreadHandle.Set();
        }
    }

    private static void DoWork(byte[] context)
    {
        Console.WriteLine("Doing 5 seconds worth of work on data");
        DateTime futureTime = DateTime.Now.AddSeconds(5);
        while (DateTime.Now < futureTime)
        { }
    }
  }
}

It seems like you are not using an IDE since most of the stuff here is marked as a warning in an IDE like Visual Studio. And you should consider reading up some Design like the SOLID principles for object oriented design, and design your code more like an API. You are writing very much Procedural instead of Object oriented code.

I am sorry that I can not comment on the implementation of the Code and the networking since I am not familiar with networking but I am sure someone else will create another answer to this. I am still kind of new to all of this so take this with a grain of salt.

As a sidenote at the end: Base_TCP_Server is not a very good namespace name.

\$\endgroup\$
  • 4
    \$\begingroup\$ This is a very nice first answer. Welcome to codereview! \$\endgroup\$ – яүυк Jun 15 '17 at 20:38
  • \$\begingroup\$ Indeed. If you could add the quote style to OP's code it would be easier to distinguish it from the suggestions ;-) \$\endgroup\$ – t3chb0t Jun 15 '17 at 20:50
  • \$\begingroup\$ Thanks :) Hoping someone can create a more technical answer that is going to look at the networking and robustness of the code. & I will be adding that @t3chb0t \$\endgroup\$ – Patrick Hollweck Jun 15 '17 at 20:50
  • \$\begingroup\$ The comments+lack of usings were since the code will be archived for future reference. Is the local Cleanup() function called at the end or would that require another call before the process function ends? The for loop in the cleanup section was to clear out the packet buffer to stop it from keeping sensitive packet data in memory, I don't think the garbage collector will clear that out. I feel that indicating the units in the timeout is important since you would have to know where it is used and the unit in the function that uses it if it isn't there (2_000_000 ms? ns?). Very nice refactor. \$\endgroup\$ – user143311 Jun 15 '17 at 22:49
  • \$\begingroup\$ Also, I can't test it because I don't have C#7 yet, but does calling Cleanup() in the else block end up running the DoWork() call, or does it make the function return? I don't see an issue with the original code's use of goto, and the local function is kind of confusing for me right now. \$\endgroup\$ – user143311 Jun 15 '17 at 23:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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