Is this thread safe?

[crankyslap]

I only just noticed that doing something like this is wrong;

int thrdcount = 0;
while (thrdcount < 4)
{
    Thread t = new Thread(() =>
    {
        dowork(thrdcount); //Start the threadWorker
    });
    t.Start();
    thrdcount++; //Increase the thread count
}

Because the loop can end up finishing so fast that all 4 created threads get a 'thrdcount' with a value of 3. Or maybe two of them get a vlue of 2 and the other two a value of 3, etc.

So I changed it to this;

int thrdcount = 0;
while (thrdcount < 4)
{
    using (ManualResetEvent waitForThreadStart = new ManualResetEvent(false)) //Allows us to wait until the thread succesfully started
    {
        Thread t = new Thread(() =>
        {
            int threadcount_use = thrdcount;
            waitForThreadStart.Set(); //Let main thread know we started
            dowork(threadcount_use); //Start the threadWorker
        });
        t.Start();
        waitForThreadStart.WaitOne(); //Wait until the thread started
        thrdcount++; //Increase the thread count
    }
}

Work great! But now I'm wondering about the way I have a server accept new connections;

/// <summary>
/// Will accept connections until GlobalVars.mustClose is true or an exception occurs
/// </summary>
public void start()
{
    try
    {
        serverSocket = new TcpListener(port);
        serverSocket.Start();

        while (true)
        {
            TcpClient clientSocket = null; //Initiate TcpClient object that we will be sending to the client handler.

            if (GlobalVars.mustClose) break;
            try
            {
                bool cont = true;
                try
                {
                    output("Waiting for client to connect.");
                    clientSocket = serverSocket.AcceptTcpClient(); //Accept new client
                    if (clientConnect == null)
                    {
                        client_error("A client handler has not been set.");
                        continue;
                    }

                    //Get conn info
                    System.Net.Sockets.Socket s = clientSocket.Client;
                    string Cip = ((IPEndPoint)s.RemoteEndPoint).Address.ToString();
                    int Cport = ((IPEndPoint)s.RemoteEndPoint).Port;

                    //Output conn info
                    string client_con = String.Format("Client connected : {0}:{1}", Cip, Cport);
                    client_error(client_con);
                }
                catch (Exception) { cont = false; } //TODO: Proper error handling
                if (cont)
                {
                    if (!GlobalVars.mustClose)
                    {
                        Thread t = new Thread(() => 
                        {
                            clientConnect(clientSocket);
                        });
                        t.Start();
                    }
                }
            }
            catch (Exception) { client_error("Exception when connecting to client."); /*Failed to connect client*/ }
        }

        //This part of the code should never be reached...
        try
        {
            serverSocket.Stop();
        }
        catch (Exception) { } //Catch exception in case it already stopped earlier
    }
    catch (Exception) { fatal_error("Failed to start."); /*Failed to start*/ }
    fatal_error("Server ended.");
}

If the while loop accepts two clients really fast, is it possible for the 'clientConnect(clientSocket);' in the first loop to send the wrong (duplicate of second or null-valued) clientSocket, or will it at all times send the correct one, since a new one is created each loop?

[Momerath]

You'll be fine. The first example is dealing with capture variables and threads. The second example doesn't deal with threads at all.

[crankyslap]

How do you mean 'It doesn't deal with threads'? It's starting a new thread after each client connect, passing the TcpClient object as an argument.

[skatamatic]

I've noticed that same problem before, too. But I don't think you rational behind why it occurs makes sense - how fast or how slow the threads fire shouldn't have an impact on the argument passed to the dowork() method.

I think it's a bug with the way annonymous delegates are optimized. To avoid that problem, I usually use the built in ParameterizedThreadStart class and pass it an object that contains the necessary information. In the first example, putting a Thread.Sleep(1) in the loop will likely correct the behaviour (it did in my test). This might be an explanation as to why your server works correctly - there's enough time in between each client connection to avoid the 'bug'.

That's just something I've noticed, it could be a complete lie with a really good explanation behind it. Regardless, it's kind of annoying.