Simplest way to make a whole method thread-safe?

Question

There seems to be a lot to learn about multithreaded programming and it's all a bit intimidating.

For my current needs, I just want to protect against a method being called again from another thread before it finishes, and my question is:

Is this an adequate (safe) way to make a method thread-safe?

class Foo
{
    bool doingWork;
    void DoWork()
    {
        if (doingWork)  // <- sophistocated thread-safety
            return;     // <-

        doingWork = true;

        try
        {
            [do work here]
        }
        finally
        {
            doingWork = false;
        }
    }
}

If that isn't sufficient, what is the simplest way to achieve this?

---

EDIT: More info about the scenario:

• There is only one instance of Foo

• Foo.DoWork() will be called from a ThreadPool thread on the Elapsed event of a System.Timers.Timer.

• Normally Foo.DoWork() will finish eons before the next time it's called, but I want to code for the slim chance that it will run long, and get called again before finishing.

---

(I'm also not smart enough to be sure if this question could be tagged language-agnostic, so I haven't. Enlightened readers, feel free to do so if applicable.)

Answer 1

Your code is not thread safe. You should use the lock keyword instead.

In your current code:

  if (doingWork)
        return;

  // A thread having entered the function was suspended here by the scheduler.

  doingWork = true;

When the next thread comes through, it will also enter the function.

This is why the lock construct should be used. It basically does the same as your code, but without the risk for a thread being interrupted in the middle:

class Foo
{
    object lockObject = new object;
    void DoWork()
    {
        lock(lockObject)
        {
            [do work here]
        }
    }
}

Note that this code has somewhat different semantics than your original. This code will cause the second thread entering to wait and then do the work. Your original code made the second thread just abort. To come closer to your original code, the C# lock statement cannot be used. The underlying Monitor construct has to be used directly:

class Foo
{
    object lockObject = new object;
    void DoWork()
    {
        if(Monitor.TryEnter(lockObject))
        {
            try
            {
                [do work here]
            }
            finally
            {
                Monitor.Exit(lockObject);
            }
        }
    }
}

Answer 2

Re-entrancy has nothing to do with multi-threading.

A re-entrant method is a method that can end up being called from within itself, on the same thread.
For example, if a method raises an event, and the client code that handles that event calls the method again inside the event handler, that method is re-entrant.
Protecting that method from re-entrancy means making sure that if you call it from inside itself, it will either not do anyhting or throw an exception.

Your code is protected from re-entrancy within the same object instance, as long as everything is on the same thread.

Unless [do work here] is capable of running external code (eg, by raising an event, or by calling a delegate or method from something else), it isn't re-entrant in the first place.

Your edited question indicates that this entire section is irrelevant to you.
You should probably read it anyway.

---

You may be (EDIT: are) looking for exclusivity – ensuring that the method will not run twice at once if called by multiple threads simultaneously.
Your code is not exclusive. If two threads run the method at once, and they both run the if statement at once, they will both get past the if, then both set the doingWork flag, and will both run the entire method.

To do that, use the lock keyword.

Answer 3

if you want easy code and dont care about performance too much it can be as easy as

class Foo
{
    bool doingWork;
object m_lock=new object();
    void DoWork()
    {
        lock(m_lock) // <- not sophistocated multithread protection
{
        if (doingWork)  
            return;     
         doingWork = true;
}


        try
        {
            [do work here]
        }
        finally
        {
lock(m_lock) //<- not sophistocated multithread protection
{
            doingWork = false;
}
        }
    }

}

If you want to incapsulate locking a little you can create a property that is thread safe like this:

public bool DoingWork
{
get{ lock(m_Lock){ return doingWork;}}
set{lock(m_lock){doingWork=value;}}
}

Now you can use it instead field , however it will result in more time spent for locking cause number of lock uses increases.

Or you can use full fence approach ( from great threading book Joseph Albahari online threading )

class Foo
{
  int _answer;
  bool _complete;

  void A()
  {
    _answer = 123;
    Thread.MemoryBarrier();    // Barrier 1
    _complete = true;
    Thread.MemoryBarrier();    // Barrier 2
  }

  void B()
  {
    Thread.MemoryBarrier();    // Barrier 3
    if (_complete)
    {
      Thread.MemoryBarrier();       // Barrier 4
      Console.WriteLine (_answer);
    }
  }
}

He states that full fence is 2x faster than lock statement. In some cases you can improve performance by removing unneeded calls to MemoryBarrier(), but using lock is simple, more clear and less error prone.

I believe this can also be done using Interlocked class around int based doingWork field.

Answer 4

http://msdn.microsoft.com/en-us/library/system.threading.barrier.aspx

Might want to look into using barrier instead, it does all of the work for you. It's the standard way of controlling reentrant code. Also allows you to control the amount of threads doing that work at once (if you allow more than 1).