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.
