How to: Write a thread-safe method that may only be called once?

Question

I'm attempting to write a thread-safe method which may only be called once (per object instance). An exception should be thrown if it has been called before.

I have come up with two solutions. Are they both correct? If not, what's wrong with them?

1. With lock:

   public void Foo()
   {
       lock (fooLock)
       {
           if (fooCalled) throw new InvalidOperationException();
           fooCalled = true;
       }
       …
   }
   private object fooLock = new object();
   private bool fooCalled;
   

2. With Interlocked.CompareExchange:

   public void Foo()
   {
       if (Interlocked.CompareExchange(ref fooCalled, 1, 0) == 1)
           throw new InvalidOperationException();
       …
   }
   private int fooCalled;
   

   If I'm not mistaken, this solution has the advantage of being lock-free (which seems irrelevant in my case), and that it requires fewer private fields.

I am also open to justified opinions which solution should be preferred, and to further suggestions if there's a better way.

Answer 1

Your Interlocked.CompareExchange solution looks the best, and (as you said) is lock-free. It's also significantly less complicated than other solutions. Locks are quite heavyweight, whereas CompareExchange can be compiled down to a single CAS cpu instruction. I say go with that one.

Answer 2

The double checked lock patter is what you are after:

This is what you are after:

class Foo
{
   private object someLock = new object();
   private object someFlag = false;


  void SomeMethod()
  {
    // to prevent locking on subsequent calls         
    if(someFlag)
        throw new Exception();

    // to make sure only one thread can change the contents of someFlag            
    lock(someLock)
    {
      if(someFlag)
        throw new Exception();

      someFlag = true;                      
    }

    //execute your code
  }
}

In general when exposed to issues like these try and follow well know patters like the one above.
This makes it recognizable and less error prone since you are less likely to miss something when following a pattern, especially when it comes to threading.
In your case the first if does not make a lot of sense but often you will want to execute the actual logic and then set the flag. A second thread would be blocked while you are executing your (maybe quite costly) code.

About the second sample:
Yes it is correct, but don't make it more complicated than it is. You should have very good reasons to not use the simple locking and in this situation it makes the code more complicated (because Interlocked.CompareExchange() is less known) without achieving anything (as you pointed out being lock less against locking to set a boolean flag is not really a benefit in this case).

Answer 3

    Task task = new Task((Action)(() => { Console.WriteLine("Called!"); }));
    public void Foo()
    {
        task.Start();
    }

    public void Bar()
    {
        Foo();
        Foo();//this line will throws different exceptions depends on 
              //whether task in progress or task has already been completed
    }