C# TcpClient/Listener Source Review.

[C#] TcpClient/Listener Source Review. - Printable Version

+- Hack Forums (https://hackforums.net)
+-- Forum: Tech (https://hackforums.net/forumdisplay.php?fid=444)
+--- Forum: Coding (https://hackforums.net/forumdisplay.php?fid=151)
+---- Forum: Software Development (https://hackforums.net/forumdisplay.php?fid=118)
+---- Thread: [C#] TcpClient/Listener Source Review. (/showthread.php?tid=5367006)

TcpClient/Listener Source Review. - OperationFatSarah - Aug 7, 2016

Hi. Just looking for someone to review my code and give some criticism. Everything works fine as far as I can tell, just wondering if I've missed anything.

http://pastebin.com/5yw60P5z

Couldn't paste the code here. Was getting a cloudflare blocking error :(

* - 6e756c6c - Aug 7, 2016

Just skimming over it but I see a few issues:

When you accept a new connection and begin reading data for it, you're creating a new thread which is going to tank performance and the max number of connections very quickly. You should really be using ReadAsync here which will handle multiple clients on fewer threads.

Code 
Thread DataThread = new Thread(tThread);

DataThread.Start();

Additionally, I see you are using ReadAsync in the DataThread, so you're basically doubling up on thread resources which is going to use a crazy amount of resources. You're also handling all data as ASCII strings which is an issue for a number of reasons, you should consider binary serialization or if you're set on strings then UTF8 at least.

Code 
DataLength = await CStream.ReadAsync(Cl.Buffer, 0, Cl.Buffer.Length);

                    Cl.DataQueString.Add(Encoding.ASCII.GetString(Cl.Buffer, 0, DataLength));

                    Cl.DataQueBuffer.Add(Cl.Buffer);

                    IncomingData((object)Cl, null);

You also have a number of Thread.Sleeps which is going to be a bottleneck once you start sending a lot of data.

I have to go for now, but I'll continue looking at it later - saw some issues with your ID generation, messbagebox in connection event, etc. which I'll address when I return.

* - ISUCKWITHCLICKING - Aug 7, 2016

In other words, Fire+Rain, you .NET script kiddie, use the IOCP Model, this is what is meant by using asynchronous sockets in .NET...
It utilizes thread pools and usermode callback from the kernel. It's the perfect threading model there is for windows.

* - Sakuto - Aug 7, 2016

Also, consider posting your code on github next time, easier than a big ass pastebin file.

* - 0xDEAD10CC - Aug 9, 2016

You're mixing raw threading with the TPL design paradigm which is a terrible idea, in addition to the fact that you're using the Random class wrong, not keeping a reference to your raw threads for management, promoting boxing/unboxing for no reason, among other things.