Strange infinite loop in thread in c# using network sockets

Question

I made a little experiment where a server send continually a message to a client until a second client be connected. Testing with telnet, the first client receive the message continually, but when the second client connects, the client counter (nClients) increments in the second client thread, but not in the first one. So with a third and a fourth clients an so on. Variable nClients still being 1 in the first client but is incremented in the next ones. The code:

 class Program
{
    volatile static int nClients = 0;
    static void clientThread(object socket)
    {

        Socket client = (Socket)socket;
        IPEndPoint ieCliente = (IPEndPoint)client.RemoteEndPoint;
        Console.WriteLine("IP: {0} port: {1}", ieCliente.Address, ieCliente.Port);

        nClients++;

        Console.WriteLine(nClients);
        using (NetworkStream ns = new NetworkStream(client))
        using (StreamWriter sw = new StreamWriter(ns))
        {
            sw.WriteLine("Welcome");
            sw.Flush();

            while (nClients < 2)
            {
                sw.WriteLine($"Waiting clients... now {nClients} connected");
                sw.Flush();
                //Thread.Sleep(100);
            }
            sw.WriteLine($"{nClients} clients");
            sw.Flush();

        }
        client.Close();
    }

    static void Main(string[] args)
    {
        int port = 31416;
        IPEndPoint ie = null;
        ie = new IPEndPoint(IPAddress.Any, port);
        using (Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
        {
            s.Bind(ie);
            s.Listen(5);
            Console.WriteLine($"Server listening at port: {ie.Port}");
            while (true)
            {
                Socket cliente = s.Accept();
                Thread th= new Thread(clientThread);
                th.Start(cliente);
            }
        }

    }
}

If I add an Thread.Sleep() into the loop (or even a Console.Writeline()) it works. Why does the first thread not detect the change of nClients.

Actualization: Following the feeedback recommendations, I was trying de Interlocked.Increment() and Volatile.Read() but the problem persist. My code again with these changes.

class Program
{
    static int nClients = 0;
    static void clientThread(object socket)
    {

        Socket client = (Socket)socket;
        IPEndPoint ieCliente = (IPEndPoint)client.RemoteEndPoint;
        Console.WriteLine("IP: {0} port: {1}", ieCliente.Address, ieCliente.Port);

        Interlocked.Increment(ref nClients);

        Console.WriteLine(Volatile.Read(ref nClients));
        using (NetworkStream ns = new NetworkStream(client))
        using (StreamWriter sw = new StreamWriter(ns))
        {
            sw.WriteLine("Welcome");
            sw.Flush();

            while (Volatile.Read(ref nClients) < 2)
            {
                sw.WriteLine($"Waiting clients... now {Volatile.Read(ref nClients)} connected");
                sw.Flush();
                //Thread.Sleep(100);
            }
            sw.WriteLine($"{Volatile.Read(ref nClients)} clients");
            sw.Flush();

        }
        client.Close();
    }

    static void Main(string[] args)
    {
        int port = 31416;
        IPEndPoint ie = null;
        ie = new IPEndPoint(IPAddress.Any, port);
        using (Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
        {
            s.Bind(ie);
            s.Listen(5);
            Console.WriteLine($"Server listening at port: {ie.Port}");
            while (true)
            {
                Socket cliente = s.Accept();
                Thread th = new Thread(clientThread);
                th.Start(cliente);
            }
        }

    }
}

And here a video showing this behavior. And a bonus: when I close abruptly the server, sometimes the client continue in the loop. I think that for some reason when I close the server, Windows close the process but the threads memory is not released and continue running.

Answer 1

The problem is here:

 nClients++;

This is being executed by more than one thread simultaneously, and it is not threadsafe.

The reason why is as follows. The code is (logically) equivalent to the following:

int temp = nClients + 1; // [A]
nClients = temp;         // [B]

So lets imagine that nClients is 0. Thread #1 executes line [A] and thus temp is 1. Now imagine that before thread #1 has executed [B], thread #2 comes along and executes [A]. Thread #2's value for temp is also 1, because nothing has changed the value of nClients yet.

Now both threads continue, and they both set nClients to temp, which is 1.

Oops! An increment went missing.

That's what's happening. (Well, this is actually a gross simplification, but it's a good way to understand how it can go wrong.)

To fix it you can either put a lock around the incrementing code, or (simpler and more efficient) use Interlocked.Increment():

Interlocked.Increment(ref nClients);