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.

Psychopharmacology

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.

Marjorymarjy

Out of curiosity: When you say that it's "less complicated", you seem to refer to all that's going on behind the blinds; how would you judge readability / ease of understanding of the Interlocked.ExchangeCompare construct for an average programmer? – Psychopharmacology

@stakx: that's what comments are for. When a programmer encounters something they don't understand, they should look it up so they do understand it. That's how they become a better programmer. – Marjorymarjy

@Marjorymarjy I disagree, yes it's the most correct solution, but it is not simple, you need to know about atomic operations etc. This is somehow an initialization process and I would advise to follow the initialization patterns that are widely used (e.g. double checked lock). Also these patterns prevent you from missing something which can easily happen when doing threading. – Tibia

This sort of thing is what CompareExchange is designed for. It uses one variable, compiles down to one CPU instruction, and is one line of code. As long as there's a comment, and no other threading issues complicating it, then there is no reason not to use it. If any of your programmers don't understand what it does then they should look it up. You shouldn't dumb down to their level, they should rise up to yours. If they don't understand about atomic operations, then they should learn. Thats how they get better, which is something everyone should aspire to do. – Marjorymarjy

Is there any advantage to using CompareExchange as opposed to plain old Exchange? The compare seems unnecessary. – Farcical

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

Tibia

1. This seems to make reading/writing someFlag non-atomic. Are you sure this is correct? 2. What about the Interlocked.CompareExchange-based solution? – Psychopharmacology

sry forgot the most important line – Tibia

Hmm... is locking such an issue if you're going to throw an exception, anyway? – Psychopharmacology

@stakx notice that you need to make the lock + flag static, otherwise another instance of the object can execute the code again – Tibia

@stakx I simply followed the double check lock pattern, the exception was your requirement. The idea is to follow a pattern so that other people will be able to understand quickly what you have done. There is often a trade off between optimized <-> maintainable my advice go with the patterns. – Tibia

That's fine, although I suppose it would be better to state the reason for the double-check in your answer, rather than in the fine print beneath it. :) -- Concerning your comment before the last, I've forgotten to mention that I didn't have a static method in mind, so one call per object instance is OK. I've updated my question. – Psychopharmacology

@stakx also the first if is necessary when you want to make sure the code was executed successfully, because then the execution of the actual logic happens inside the lock and the last thing you do is set the flag, you did not mention whether this is necessary in your case – Tibia

Let's not go too far with this discussion, please. Concerning your last point, I actually said exactly what I meant: that the method may not be called more than once. – Psychopharmacology

@stakx sure thing :) I have extended my answer to include point's raised in the comments for future readers. – Tibia

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
    }    

Interregnum

Sorry to say that this does not answer the question. (Btw., I'm aware of the Task Parallel Library, but it cannot be used everywhere, e.g. where the method in question implements an interface method.) – Psychopharmacology