Is this a bad practice with static classes and thread safety?

Question

Assuming I'm not using the lock statement, is this a bad practice?

public static class Foo
{
  public static string Bar()
  {
    bool working;

    while (working)
    {
      // Loop until ready to run.
    }

    working = true;

    // Do stuff here.

    working = false;
    return "done."
  }
}

Edit -

After trying something like this, I realize it's not feasible for several reasons. I was just curious.. The example I posted doesn't even work.

Answer 1

Loop is a CPU consuming process.

I mean if everything you do is just waiting. It is not a good.

Answer 2

First off, working is private to the method, so it will have no effect. 2 threads calling this will each have their own "working". You'd need to make it a static field to have an effect.

Even then, you can get race conditions. 2 threads can hit this at the same time, pass through the while(working) condition, and then set working = true; and be doing things at the same time.

Using a lock or a semaphore will help solve this.

Answer 3

Assuming you mean that working will be modified from another thread, there are two problems:

1. You should make working into a volatile static member. Volatile will tell the compiler not to try anything "clever" in optimization.

2. You've written what is called a "spin lock". There are specialized circumstances where that is the best approach. But usually it's a terrible idea because your thread will consume CPU until the working variable is set.

Answer 4

Yes, this is bad practice.

Why would you not using the lock statement? Look at the Monitor class, I can provide mutual exclusion and synchronization.

The spinning loop and non-threadsafe variable working shouldn't be used.

Answer 5

There is another issue that if working were visible to multiple threads (as another poster mentioned, it is a local variable), as written right now there is no guarantee that other cores in the system will see the updates to working in the right order. A memory barrier, such as System.Threading.Thread.MemoryBarrier, is needed to guarantee that the write to working doesn't get re-ordered by the CPU past "Do stuff here."

But you should consider using locks instead. Lock-free programming is incredibly hard to get right.

Answer 6

Using locking is the only way to prevent concurrency problems (other that not doing it).

public static class Foo
{
  private static object barLock = new object();

  public static string Bar()
  {
    lock (barLock)
    {
      // Do work
      return "Done";
    }
  }
}