Is the following C# code thread safe?

I am trying to learn the th开发者_运维百科reading in C#. Today I sow the following code at http://www.albahari.com/threading/:

class ThreadTest
{
  bool done;

  static void Main()
  {
    ThreadTest tt = new ThreadTest();   // Create a common instance
    new Thread (tt.Go).Start();
    tt.Go();
  }

  // Note that Go is now an instance method
  void Go() 
  {
     if (!done) { done = true; Console.WriteLine ("Done"); }
  }
}

In Java unless you define the "done" as volatile the code will not be safe. How does C# memory model handles this?

Guys, Thanks all for the answers. Much appreciated.

---

Well, there's the clear race condition that they could both see done as false and execute the if body - that's true regardless of memory model. Making done volatile won't fix that, and it wouldn't fix it in Java either.

But yes, it's feasible that the change made in one thread could happen but not be visible until in the other thread. It depends on CPU architecture etc. As an example of what I mean, consider this program:

using System;
using System.Threading;

class Test
{
    private bool stop = false;

    static void Main()
    {
        new Test().Start();
    }

    void Start()
    {
        new Thread(ThreadJob).Start();
        Thread.Sleep(500);
        stop = true;
    }

    void ThreadJob()
    {
        int x = 0;
        while (!stop)
        {
            x++;
        }
        Console.WriteLine("Counted to {0}", x);
    }
}

While on my current laptop this does terminate, I've used other machines where pretty much the exact same code would run forever - it would never "see" the change to stop in the second thread.

Basically, I try to avoid writing lock-free code unless it's using higher-level abstractions provided by people who really know their stuff - like the Parallel Extensions in .NET 4.

There is a way to make this code lock-free and correct easily though, using Interlocked. For example:

class ThreadTest
{
  int done;

  static void Main()
  {
    ThreadTest tt = new ThreadTest();   // Create a common instance
    new Thread (tt.Go).Start();
    tt.Go();
  }

  // Note that Go is now an instance method
  void Go() 
  {
     if (Interlocked.CompareExchange(ref done, 1, 0) == 0) 
     {
         Console.WriteLine("Done");
     }
  }
}

Here the change of value and the testing of it are performed as a single unit: CompareExchange will only set the value to 1 if it's currently 0, and will return the old value. So only a single thread will ever see a return value of 0.

Another thing to bear in mind: your question is fairly ambiguous, as you haven't defined what you mean by "thread safe". I've guessed at your intention, but you never made it clear. Read this blog post by Eric Lippert - it's well worth it.

---

No, it's not thread safe. You could potentially have one thread check the condition (if(!done)), the other thread check that same condition, and then the first thread executes the first line in the code block (done = true).

You can make it thread safe with a lock:

lock(this)
{
    if(!done)
    {
        done = true;
        Console.WriteLine("Done");
    }
}

---

Even in Java with volatile, both threads could enter the block with the WriteLine.

If you want mutual exclusion you need to use a real synchronisation object such as a lock.

---

onle way this is thread safe is when you use atomic compare and set in the if test

if(atomicBool.compareAndSet(false,true)){
   Console.WriteLine("Done");
}

---

You should do something like this:

class ThreadTest{
Object myLock = new Object();
...
void Go(){
  lock(myLock){
     if(!done)
     {
         done = true;
         Console.WriteLine("Done");
     }
   }
}

The reason you want to use an generic object, rather than "this", is that if your object (aka "this") changes at all it is considered another object. Thus your lock does not work any more.

Another small thing you might consider is this. It is a "good practices" thing, so nothing severe.

class ThreadTest{
Object myLock = new Object();
...
void Go(){
  lock(myLock){
     if(!done)
     {
         done = true;
     }
   }
   //This line of code does not belong inside the lock.
   Console.WriteLine("Done");
}

Never have code inside a lock that does not need to be inside a lock. This is due to the delay this causes. If you have lots of threads you can gain a lot of performance from removing all this unnecessary waiting.

Hope it helps :)