6

I am trying to learn the threading 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.

Improve this question
6
  • Why does this question have a java tag, I know it references how it could be done in java, but it is strictly a C# question. Commented May 12, 2011 at 19:53
  • I doubt that declaring done as volatile in Java would prevent "Done" from being printed twice in some circumstances. Just because you have a memory fence pre-read doesn't mean that the first thread to find done == false can't be pre-empted by the other thread. Commented May 12, 2011 at 19:54
  • @Robin As you said because I wander how it differs than Java. But you are right this is a C# question. Do you know how to remove? Commented May 12, 2011 at 19:55
  • @Fuad Malikov. I removed it for you :) Commented May 12, 2011 at 19:56
  • C# has volatile as well. You should read the rest of that article for more info on lock, though Commented May 12, 2011 at 19:59

5 Answers 5

Reset to default
7

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

is Interlocked.CompareExchange works in a similar way as if(atomicBool.compareAndSet(false,true)){ Console.WriteLine("Done"); } as ratchet freak (down in the answers) suggested. Thanks for the link btw.
@Fuad: I know nothing about the implementation behind the two, but they're the same approach, yes.
5

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"); } }
Improve this answer

1 Comment

Though using a lock is an excellent idea, "lock this" is a bad programming practice. See stackoverflow.com/questions/5926393/… for details.
3

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.

Improve this answer

Comments

1

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"); }
Improve this answer

Comments

0

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 :)

Improve this answer

1 Comment

I would disagree with moving the WriteLine, since the intent appears to be that "Done" only be printed if done == false. Good point in general, though.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.