C# TCP/IP simple chat with multiple-clients

Question

I'm learning c# socket programming. So, I decided to make a TCP chat, the basic idea is that A client send data to the server, then the server broadcast it for all the clients online (in this case all the clients are in a dictionary).

When there is 1 client connected, it works as expected, the problem is occurred when there is more than 1 client connected.

Server:

class Program
{
    static void Main(string[] args)
    {
        Dictionary<int,TcpClient> list_clients = new Dictionary<int,TcpClient> ();

        int count = 1;


        TcpListener ServerSocket = new TcpListener(IPAddress.Any, 5000);
        ServerSocket.Start();

        while (true)
        {
            TcpClient client = ServerSocket.AcceptTcpClient();
            list_clients.Add(count, client);
            Console.WriteLine("Someone connected!!");
            count++;
            Box box = new Box(client, list_clients);

            Thread t = new Thread(handle_clients);
            t.Start(box);
        }

    }

    public static void handle_clients(object o)
    {
        Box box = (Box)o;
        Dictionary<int, TcpClient> list_connections = box.list;

        while (true)
        {
            NetworkStream stream = box.c.GetStream();
            byte[] buffer = new byte[1024];
            int byte_count = stream.Read(buffer, 0, buffer.Length);
            byte[] formated = new Byte[byte_count];
            //handle  the null characteres in the byte array
            Array.Copy(buffer, formated, byte_count);
            string data = Encoding.ASCII.GetString(formated);
            broadcast(list_connections, data);
            Console.WriteLine(data);

        } 
    }

    public static void broadcast(Dictionary<int,TcpClient> conexoes, string data)
    {
        foreach(TcpClient c in conexoes.Values)
        {
            NetworkStream stream = c.GetStream();

            byte[] buffer = Encoding.ASCII.GetBytes(data);
            stream.Write(buffer,0, buffer.Length);
        }
    }

}
class Box
{
    public TcpClient c;
     public Dictionary<int, TcpClient> list;

    public Box(TcpClient c, Dictionary<int, TcpClient> list)
    {
        this.c = c;
        this.list = list;
    }

}

I created this box, so I can pass 2 args for the Thread.start().

Client:

class Program
{
    static void Main(string[] args)
    {
        IPAddress ip = IPAddress.Parse("127.0.0.1");
        int port = 5000;
        TcpClient client = new TcpClient();
        client.Connect(ip, port);
        Console.WriteLine("client connected!!");
        NetworkStream ns = client.GetStream();

        string s;
        while (true)
        {
             s = Console.ReadLine();
            byte[] buffer = Encoding.ASCII.GetBytes(s);
            ns.Write(buffer, 0, buffer.Length);
            byte[] receivedBytes = new byte[1024];
            int byte_count = ns.Read(receivedBytes, 0, receivedBytes.Length);
            byte[] formated = new byte[byte_count];
            //handle  the null characteres in the byte array
            Array.Copy(receivedBytes, formated, byte_count); 
            string data = Encoding.ASCII.GetString(formated);
            Console.WriteLine(data);
        }
        ns.Close();
        client.Close();
        Console.WriteLine("disconnect from server!!");
        Console.ReadKey();        
    }
}

Answer 1

It is not clear from your question what problems specifically it is you are having. However, inspection of the code reveals two significant problems:

1. You do not access your dictionary in a thread-safe way, which means that the listening thread, which may add items to the dictionary, can operate on the object at the same time that a client-serving thread is trying to examine the dictionary. But, the add operation is not atomic. Meaning that during the course of adding an item, the dictionary may temporarily be in an invalid state. This would cause problems for any client-serving thread that is trying to read it concurrently.
2. Your client code attempts to process the user input and writes to the server in the same thread that is handling receiving data from the server. This can result in at least a couple of problems:
   • It is not possible to receive data from another client until the next time the user provides some input.
   • Because you may receive as little as a single byte in a single read operation, even after the user provides input, you may still not receive the complete message that was sent previously.

Here is a version of your code that addresses these two issues:

Server code:

class Program
{
    static readonly object _lock = new object();
    static readonly Dictionary<int, TcpClient> list_clients = new Dictionary<int, TcpClient>();

    static void Main(string[] args)
    {
        int count = 1;

        TcpListener ServerSocket = new TcpListener(IPAddress.Any, 5000);
        ServerSocket.Start();

        while (true)
        {
            TcpClient client = ServerSocket.AcceptTcpClient();
            lock (_lock) list_clients.Add(count, client);
            Console.WriteLine("Someone connected!!");

            Thread t = new Thread(handle_clients);
            t.Start(count);
            count++;
        }
    }

    public static void handle_clients(object o)
    {
        int id = (int)o;
        TcpClient client;

        lock (_lock) client = list_clients[id];

        while (true)
        {
            NetworkStream stream = client.GetStream();
            byte[] buffer = new byte[1024];
            int byte_count = stream.Read(buffer, 0, buffer.Length);

            if (byte_count == 0)
            {
                break;
            }

            string data = Encoding.ASCII.GetString(buffer, 0, byte_count);
            broadcast(data);
            Console.WriteLine(data);
        }

        lock (_lock) list_clients.Remove(id);
        client.Client.Shutdown(SocketShutdown.Both);
        client.Close();
    }

    public static void broadcast(string data)
    {
        byte[] buffer = Encoding.ASCII.GetBytes(data + Environment.NewLine);

        lock (_lock)
        {
            foreach (TcpClient c in list_clients.Values)
            {
                NetworkStream stream = c.GetStream();

                stream.Write(buffer, 0, buffer.Length);
            }
        }
    }
}

Client code:

class Program
{
    static void Main(string[] args)
    {
        IPAddress ip = IPAddress.Parse("127.0.0.1");
        int port = 5000;
        TcpClient client = new TcpClient();
        client.Connect(ip, port);
        Console.WriteLine("client connected!!");
        NetworkStream ns = client.GetStream();
        Thread thread = new Thread(o => ReceiveData((TcpClient)o));

        thread.Start(client);

        string s;
        while (!string.IsNullOrEmpty((s = Console.ReadLine())))
        {
            byte[] buffer = Encoding.ASCII.GetBytes(s);
            ns.Write(buffer, 0, buffer.Length);
        }

        client.Client.Shutdown(SocketShutdown.Send);
        thread.Join();
        ns.Close();
        client.Close();
        Console.WriteLine("disconnect from server!!");
        Console.ReadKey();
    }

    static void ReceiveData(TcpClient client)
    {
        NetworkStream ns = client.GetStream();
        byte[] receivedBytes = new byte[1024];
        int byte_count;

        while ((byte_count = ns.Read(receivedBytes, 0, receivedBytes.Length)) > 0)
        {
            Console.Write(Encoding.ASCII.GetString(receivedBytes, 0, byte_count));
        }
    }
}

Notes:

• This version uses the lock statement to ensure exclusive access by a thread of the list_clients object.
• The lock has to be maintained throughout the broadcast of messages, to ensure that no client is removed while enumerating the collection, and that no client is closed by one thread while another is trying to send on the socket.
• In this version, there is no need for the Box object. The collection itself is referenced by a static field accessible by all the methods executing, and the int value assigned to each client is passed as the thread parameter, so the thread can look up the appropriate client object.
• Both server and client watch for and handle a read operation that completes with a byte count of 0. This is the standard socket signal used to indicate that the remote endpoint is done sending. An endpoint indicates it's done sending by using the Shutdown() method. To initiate the graceful closure, Shutdown() is called with the "send" reason, indicating that the endpoint has stopped sending, but will still receive. The other endpoint, once done sending to the first endpoint, can then call Shutdown() with the reason of "both" to indicate that it is done both sending and receiving.

There are still a variety of issues in the code. The above addresses only the most glaring, and brings the code to some reasonable facsimile of a working demonstration of a very basic server/client architecture.

Addendum:

Some additional notes to address follow-up questions from the comments:

• The client calls Thread.Join() on the receiving thread (i.e. waits for that thread to exit), to ensure that after it's starting the graceful closure process, it does not actually close the socket until the remote endpoint responds by shutting down its end.
• The use of o => ReceiveData((TcpClient)o) as the ParameterizedThreadStart delegate is an idiom I prefer over the casting of the thread argument. It allows the thread entry point to remain strongly-typed. Though, that code is not exactly how I would have ordinarily written it; I was sticking closely to your original code, while still using the opportunity to illustrate that idiom. But in reality, I would use the constructor overload using the parameterless ThreadStart delegate and just let the lambda expression capture the necessary method arguments: Thread thread = new Thread(() => ReceiveData(client)); thread.Start(); Then, no casting at all has to happen (and if any arguments are value types, they are handled without any boxing/unboxing overhead…not usually a critical concern in this context, but still makes me feel better :) ).
• Applying these techniques to a Windows Forms project adds some complication, unsurprisingly. When receiving in a non-UI thread (whether a dedicated per-connection thread, or using one of the several asynchronous APIs for network I/O), you will need to get back to the UI thread when interacting with the UI objects. The solution that here is the same as usual: the most basic approach is to use Control.Invoke() (or Dispatcher.Invoke(), in a WPF program); a more sophisticated (and IMHO, superior) approach is to use async/await for the I/O. If you are using StreamReader to receive data, that object already has an awaitable ReadLineAsync() and similar methods. If using the Socket directly, you can use the Task.FromAsync() method to wrap the BeginReceive() and EndReceive() methods in an awaitable. Either way, the result is that while the I/O occurs asynchronously, completions still get handled in the UI thread where you can access your UI objects directly. (In this approach, you would wait on the task representing the receiving code, instead of using Thread.Join(), to ensure you don't close the socket prematurely.)