C# TCP server/client class

Question

Goal

My goal is to create a TCP server/client class that can be universally reused in various private projects. It should work stable and does not need to be super fast, but rather light-weight to even work on a Raspberry Pi, for example. One TCP connection at the time would be enough for me.

Question

Since I am somewhat new to C# and not an expert on TCP-communications, I would like to know, if the "lock-mechanism" is correctly implemented and if there is any better way of handling errors. In addition to that, I would be glad to hear general improvements to the code or my coding style.

Code

In addition to the code below, I have a link to my project on GitHub as well as a link to the testing program that I created for this class/lib. NOTE: The class is targeted to .NET Standard 2.0

TCP server/client class

Testing application

using System;
using System.Net.Sockets;
using System.Threading;
using System.Text;
using System.Net;
using System.Collections;

namespace TcpConnection_Lib
{
    public class TcpConnection : IDisposable
    {
        //fields and properties:
        private TcpClient client;
        private TcpListener listener;

        private Thread ListenThread;
        private Thread TcpReaderThread;

        public string RemoteEndpointAddress { get; private set; }

        private readonly Queue ReceivedStringQueue = new Queue();

        public bool TcpIsConnected
        {
            get
            {
                if (client != null)
                {
                    return client.Connected;
                }
                else
                {
                    return false;
                }
            }
        }

        private readonly byte[] receiveBuffer = new byte[4096];

        private readonly object syncLock = new object();


        //methods:
        public bool Connect(string IP, int port)
        {
            try
            {
                bool successFlag = false;

                lock (syncLock)
                {
                    try
                    {
                        client = new TcpClient();
                        client.Connect(IP, port);
                        client.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);

                        if (TcpReaderThread != null)
                        {
                            TcpReaderThread.Abort();
                            TcpReaderThread = null;
                        }
                        TcpReaderThread = new Thread(ReadData)
                        {
                            IsBackground = true
                        };
                        TcpReaderThread.Start();
                        successFlag = true;
                    }
                    catch { }
                }
                return successFlag;
            }
            catch
            {
                return false;
            }
        }

        public bool Disconnect()
        {
            try
            {
                lock (syncLock)
                {
                    try
                    {
                        if (TcpReaderThread != null)
                        {
                            TcpReaderThread.Abort();
                            TcpReaderThread = null;
                        }
                        if (client != null)
                        {
                            client.Client.Close();
                            client.Close();
                            client = null;
                        }
                        if (ReceivedStringQueue.Count > 0)
                        {
                            ReceivedStringQueue.Clear();
                        }
                    }
                    catch { }
                }
                return true;
            }
            catch
            {
                return false;
            }
        }

        public bool Send(string sendString)
        {
            try
            {
                bool successFlag = false;

                lock (syncLock)
                {
                    try
                    {
                        client.Client.Send(ASCIIEncoding.ASCII.GetBytes(sendString));
                        successFlag = true;
                    }
                    catch { }
                }
                return successFlag;
            }
            catch
            {
                return false;
            }
        }

        public string GetReceivedString()
        {
            try
            {
                string returnString = "";

                lock (ReceivedStringQueue.SyncRoot)
                {
                    try
                    {
                        if (ReceivedStringQueue.Count > 0)
                        {
                            returnString = ReceivedStringQueue.Dequeue().ToString();
                        }
                    }
                    catch { }
                }
                return returnString;
            }
            catch
            {
                return "";
            }
        }

        public bool Listen(int port)
        {
            try
            {
                IPEndPoint ipLocalEndPoint = new IPEndPoint(IPAddress.Any, port);
                listener = new TcpListener(ipLocalEndPoint);
                listener.Start(port);

                if (ListenThread != null)
                {
                    ListenThread.Abort();
                    ListenThread = null;
                }
                ListenThread = new Thread(ListeningMethod)
                {
                    IsBackground = true
                };
                ListenThread.Start();
                return true;
            }
            catch
            {
                return false;
            }
        }

        public void Dispose()
        {
            try
            {
                lock (syncLock)
                {
                    try
                    {
                        Disconnect();
                        if (listener != null)
                        {
                            listener.Stop();
                            listener = null;
                        }
                        if (client != null)
                        {
                            client.Close();
                            client = null;
                        }
                        if (ListenThread != null)
                        {
                            ListenThread.Abort();
                            ListenThread = null;
                        }
                        if (TcpReaderThread != null)
                        {
                            TcpReaderThread.Abort();
                            TcpReaderThread = null;
                        }
                        if (ReceivedStringQueue.Count > 0)
                        {
                            ReceivedStringQueue.Clear();
                        }
                    }
                    catch { }
                }
                GC.SuppressFinalize(this);
            }
            catch { }
        }

        private void ListeningMethod()
        {
            try
            {
                while (true)
                {
                    try
                    {
                        client = listener.AcceptTcpClient();
                        RemoteEndpointAddress = client.Client.RemoteEndPoint.ToString();
                        client.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);

                        if (TcpReaderThread != null)
                        {
                            TcpReaderThread.Abort();
                            TcpReaderThread = null;
                        }
                        TcpReaderThread = new Thread(ReadData)
                        {
                            IsBackground = true
                        };
                        TcpReaderThread.Start();
                    }
                    catch
                    {
                        if (listener != null)
                        {
                            listener.Stop();
                            listener = null;
                        }
                        break;
                    }
                }
            }
            catch { }
        }

        private void ReadData()
        {
            try
            {
                int bytesRead = 0;

                while (true)
                {
                    if (!client.Connected)
                    {
                        break;
                    }

                    bytesRead = client.GetStream().Read(receiveBuffer, 0, receiveBuffer.Length);

                    if (bytesRead == 0)
                    {
                        break;
                    }

                    CopyReceived(Encoding.ASCII.GetString(receiveBuffer, 0, bytesRead));
                }
            }
            catch { }
        }

        private void CopyReceived(string receivedData)
        {
            try
            {
                lock (ReceivedStringQueue.SyncRoot)
                {
                    try
                    {
                        ReceivedStringQueue.Enqueue(receivedData);
                    }
                    catch { }
                }
            }
            catch { }
        }
    }
}

EDIT (as VisualMelon suggested):

The API has following interfaces:

methods:

• bool Connect(string IP, int port) - returns true, if the client could connect to the server
• bool Disconnect() - returns true, if all connections could be successfully closed
• bool Listen(int port) - returns true, if the listener could be successfully started
• bool Send(string sendString) - returns true, if the string could be successfully sent
• string GetReceivedString() - returns the received string or an empty string, if nothing got received
• void Dispose() - runs Disconnect() and disposes everything

properties:

• RemoteEndpointAddress - address of the client that connected to the server
• TcpIsConnected - is true, if a TCP client is connected

A simple demonstration of a server implementation with my API:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using TcpConnection_Lib;

namespace Tcp_Lib_Server_Tester
{
    class Program
    {
        static TcpConnection connection = new TcpConnection();
        static void Main(string[] args)
        {
            connection.Listen(23); //starts a TCP listener on port 23

            while (!connection.TcpIsConnected)  //wait until a client connects
            {
                System.Threading.Thread.Sleep(100);
            }

            Console.Write("remote endpoint address: " + connection.RemoteEndpointAddress + Environment.NewLine); //prints the remote endpoint IP to the console

            bool successfulSendFlag = connection.Send("Hello world!"); //the server sends "Hello World!" to the remote peer and returns if the operation was successful

            if (successfulSendFlag)
            {
                Console.Write("String successfully sent." + Environment.NewLine);
            }
            else
            {
                Console.Write("String NOT successfully sent." + Environment.NewLine);
            }

            string tempReceiveString = "";

            while (tempReceiveString == "")
            {
                tempReceiveString = connection.GetReceivedString(); //returns the received string or an empty string, if nothing got received so far
            }

            Console.Write("Received: " + tempReceiveString + Environment.NewLine); //prints the received string to the console

            Console.ReadLine(); //keeps the console alive
        }
    }
}

(The client implementation would use "Connect(ip, port)" instead of "Listen(port)" and you could not print the "RemoteEndpointAddress".)

While writing this sample code I discovered a hidden bug. If you check the RemoteEndpointAddress exactly after the moment where a client connects to the server, then the RemoteEndpointAddress would be null (therefore I implemented the Sleep() in the first while-loop of the API-example-code). My first proposed solution would be to lock-in the "get" of the TcpIsConnected-property as well as the inner part of the while-loop in the ListeningMethod(), but this would block the TcpIsConnected-property forever since AcceptClient() is a blocking method. This means that I have to find a better way to fix this bug...

About the "lock-in" mechanism: I don't have any special use case other than preventing bugs (like the one mentioned above).

EDIT (as dfhwze suggested):

Main goals:

• learn C# and OOP
• learn about TCP
• improve my coding style and architecture
• create a universal library for any future projects that may come up, but nothing specific (theoretical uses: sending and receiving commands between single-board computers)
• create a complete bugfree, well tested and good to read code

Single client instance:

Thank you for pointing that out, because I totally overlooked this limitation!

Server or Client:

This class should either be used as a client or as a server, but not both at the same time.

Answer 1

Networking

You have done the classic thing that all people new to TCP do of assuming that whatever NetworkStream.Read gives you will be some meaningful chunk of data: this is not guaranteed, and only 'works' because you are sending tiny packages locally. Basically, if you want this to be a general-purpose and reusable system for sending discrete messages (i.e. strings), then you need to design a protocol to communicate this information. Instead of writing pages about it here, I'll just point you to an older answer of mine which discusses this in more detail. You also have a fixed-size buffer, so it's impossible at the moment to send a message longer than 4096 bytes without ending up with 2 messages: how you handle really long packets is whole other concern I won't go into.

You've done another classic thing, which is to provide a dedicated thread for reading. There a few reasons why this isn't ideal: it doesn't scale well (having hundres of blocked threads consumes memory and messes with scheduling), it means you end up managing threads (which is not fun), and it increases complexity (what do I do when the thread fails? who is responsible for killing the thread?). If you need asyncronous communication (as you have tried to implement), then you should use the asynchronous APIs on the NetworkStream (e.g. BeginRead/EndRead or ReadAsync). Running a dedicated 'read and remember' loop can be necessary/useful in some cases, but it should still use the asynchronous APIs.

Going asynchronous will also enable you to provide snappier reads to your queue. Currently a call to GetReceivedString can be blocked by pretty much anything, which means it has little use as a polling API.

Threading

Don't do this:

ListenThread.Abort();

Some people call exceptions hand-grenades, but they are wrong: exceptions are lovely and only make life better. Thread.Abort, however, is a demolition charge, and you give up a lot of confidence that anything will work if you ever do this. You have no idea what the thread was doing, and you have no idea if you are about to leave something in an invalid state. If you had some fancy lock-less concurrency going on (which currently you don't, but that may change...) you could easily lock up your entire program (which is honestly preferable to just killing a thread that was probably half way through something important).

The simplest way to resolve this problem is to abandon the behaviour of allowing multiple calls to Listen: you can't guarantee what this will do, so you shouldn't even try to support it. If someone calls Listen twice, throw and exception telling them to not do so in future, and document the behaviour precisely. One of dfhwze's comments mentioned that it is odd to provide a class which does the job of both a client and a server, and this is related: you would be much better off separating the job of being a server out into a separate class (just like TcpListener is separate from TcpClient). At the same time, you can solve your problem with the endpoint not being set by setting it in a constructor (whereafter you can make it readonly).

Even if you did have to terminate a reader, don't do it like this. Instead, kill the connection and detect this (somehow) within the read-loop if you need to close gracefully in that instance.

API and Error Handling

Your code can't send or receive empty strings. Implementing a proper protocol will solve this. For a polling GetReceiveString method, returning null when there is nothing available makes much more sense than returning "": that way you can send an empty string (if the protocol supports it), but it also has the benefit that it will crash violently if you try to use it. You also shouldn't be returning "" in GetReceivedString if there is a failure. Another options is to have a bool TryRead(out string message) method that returns a true if there was something read and sets the message accordingly, and returns false and sets message tonull` if there was not.

If there is a failure, do not swallow the exception and carry on merrily: this will only make debugging harder in the future when something does actually go wrong. For instance, I would probably remove both try...catch statements in GetReceivedString: that code should never go wrong: if it does, then you have a problem with your implementation, and you need to address it. If I was building a larger API, I might define a new Exception type which I can return when something goes unexpectedly wrong pass the exception from the outer try...catch as an inner exception.

In almost every method you swallow all the exceptions: sometimes this is 'ok', but that's only because I can see how it might map onto a carefully designed API, and I suspect that rather you are trying to make the code 'robust': this is a bad idea. If you are getting exceptions which you should be handling, then handling them specifically, and document the behaviour precisely. It's so much better for your program to crash and burn because something unexpected happened, than for it to pretend that everything is fine when it's ended up in an invalid state. Again, in much of your code you can just about get away with this, but it is not a good habit.

I can see that you want to call Listen and then just start trying to read, but this gives you nothing that calling Listen and waiting for a client to connect (which you can package in another object) before trying to read would give you.

Even if you did want some way to swap out the client without changing the reading object (which would be enough logic to warrant it's own class), then consider that fact that whoever is reading has no idea that the connection may have changed, and that it may have changed between their reading a message and writing one. This sort of thing needs a very clear set of requirements, and is not the job of a general purpose networking API.

You should throw an ArgumentNullException if Send is called with a null parameter: it will throw one anyway, but this will be swallowed and look like a general failure.

TcpIsConnected is kind-of useless, because it doesn't tell you anything about whether you will be able to read again. It could be false because you are currently swapping reader, or simply not connected yet. Again, you've jammed three complex tasks (syncing reads from multiple places; listening for clients; reading from clients), and ended up with a confusing and unpredictable API. Much of the misery goes away if you separate the Listen and Read objects, because then you can make TcpIsConnect a one-way latch: it starts true, and if it ever goes false then you've lost it forever.

TcpIsConnected should probably also be taking the SyncLock or use client?.Connected (which explicitly caches the value of client); otherwise you are depending on dodgy runtime optimisations/luck to avoid a possible NullReferenceException.

Misc

• Your class is hardcoded to use ASCII: consider making the encoding configurable, or at the very least use UTF-8, which will allow you to send any string without corruption.

• You can return from within a lock: you don't need all those returnString and successFlag variables, and I would consider getting rid of them: they make it harder for the compiler to help you, as the variables over-extend their meaningful scope.

• You should consider adding inline documentation (///) to all you public members: this commits you to writing a method that has a specific job, means that the maintainer knows what the code is meant to do, and allows someone consuming your API to interrogate it without leaving their API. Inline documentation is great.

• It's good that you are using Environment.NewLine, but it would be neater if you just used Console.WriteLine.

• Consider Console.ReadKey(true) for 'pausing': it's just a cleaner way which people come to expect. It feels wrong for the program to take your input, only to throw it away when you press return.

• You don't have (nor need) a finalizer, so it's not clear what GC.SuppressFinalize is doing for you.

• I have no idea why ListeningMethod has a while loop. It looks like it once either 'retried' or was meant to accept multiple clients, but now does not.

• It's conventional to use camel-case for all parameters, including initialisms like ip (which should probably have a clearer name).

• Avoid 'magic numbers' like 4096: perhaps the user would like this to be configurable, and how is the maintainer supposed to know how this number was chosen? Put it in a constant somewhere with a helpful name, and explain why this hard-coded value was chosen: even if it just 'seemed like a good compromise' then write that, so that you know you didn't have some important reason for choosing it you need to bear in mind.

• You might consider detecting when the object is disposed and throwing an ObjectDisposedException if e.g. someone tries to send something.

Summary

I think the best think you could do is abandon any notion of having one instance of a class do the 4+ different jobs it is current doing (the 'Single Responsibility Principle' certainly applies here). Instead, provide a class to perform (basic) duplex communications (e.g. send and receive strings) over a TcpClient, and provide independent mechanisms (could just be a single static method each) for the following:

• Listen for and accept a TcpClient on some port, and wrap it in your class
• Create and connect a TcpClient to some endpoint, and wrap it in your class

Do not even try to make your class reusable: that is just asking for misery. Instead, if you want a way to swap out clients (which is a questionable objective that needs a specification so that you know what your methods are meant to do, which is a domain problem with which we cannot help you), write another a simple class that provides this behaviour. A ConcurrentQueue<string> would probably suffice as a backing data-structure.

The protocol is a detail of the API of the TcpClient wrapper, and can be worked out independently of the overall design.

Once you have a sensible and working API, you can then worry about making it robust (i.e. handles invalid usage and network connections properly (not just swallowing all exceptions)). You can worry about making it fast when you discover that it is too slow.

Answer 2

Review

This review handles readability metrics and C# conventions only and should provide insights to reach some of your goals.

Goals: learn C#, improve coding style, create good to read code

• There are a couple of variants how to name instance variables. The most common one is to use an underscore as prefix and camel-case the name; _client, _listener, _listenThread, _tcpReaderThread, _receivedStringQueue, and so on.
• Use a generic queue when dealing with a specific type; Queue ReceivedStringQueue -> Queue<string> ReceivedDataQueue. The specifc type's name should not be part of the variable name.
• Use null-propagation where convenient. if (TcpReaderThread != null) TcpReaderThread.Abort(); -> TcpReaderThread?.Abort();
• Avoid empty catch blocks; catch { } Think about which errors to catch, how to handle them, logging, rethrowing, invalidating state, etc.
• Avoid redundant checks; if (ReceivedStringQueue.Count > 0) ReceivedStringQueue.Clear(); -> ReceivedStringQueue.Clear(); Clear does not care what the count is.
• It's a convention to prefix method names with Try when you return a boolean whether or not the method call was a sucess; bool Send(string sendString) -> bool TrySend(string sendString).
• The type of member should not be part of a member name; ListeningMethod

Answer 3

while (true)

Other answers cover most things although as a small point I'd consider using a Wait Handle instead of a while loop

https://docs.microsoft.com/en-us/dotnet/api/system.threading.manualresetevent?view=netframework-4.8