Interlocked.exchange as a lock

Question

I'm trying to use Interlocked.Exchange to create a thread safe lock for some object initialize functions. Consider the below code. I want to be sure that the if is doing the same as when the while is substituted. The reason I ask is if the code is run over and over there are times when you get an exit message before the set message. I just would like to confirm that this is just a gui thing since the state on exit always seems to be correct.

class Program
{
    private static void Main(string[] args)
    {
        Thread thread1 = new Thread(new ThreadStart(() => InterlockedCheck("1")));
        Thread thread2 = new Thread(new ThreadStart(() => InterlockedCheck("2")));
        Thread thread3 = new Thread(new ThreadStart(() => InterlockedCheck("3"))); 
        Thread thread4 = new Thread(new ThreadStart(() => InterlockedCheck("4")));
        thread4.Start();
        thread1.Start();
        thread2.Start();
        thread3.Start();

        Console.ReadKey();
    }

    const int NOTCALLED = 0;
    const int CALLED = 1;
    static int _state = NOTCALLED;
    //...
    static void InterlockedCheck(string thread)
    {
        Console.WriteLine("Enter thread [{0}], state [{1}]", thread, _state);

        //while (Interlocked.Exchange(ref _state, CALLED) == NOTCALLED)
        if (Interlocked.Exchange(ref _state, CALLED) == NOTCALLED)
        {
            Console.WriteLine("Setting state on T[{0}], state[{1}]", thread, _state);
        }

        Console.WriteLine("Exit from thread [{0}] state[{1}]", thread, _state);
    }
}

i3arnon · Answer 1 · 2014-12-19T19:43:18.933

I wouldn't call that a lock since it can be used only once, but you are correct if you assume that the statements inside the if scope would be executed exactly once even if InterlockedCheck is called from multiple threads concurrently.

That's because you're starting with NOTCALLED and only setting CALLED using the atomic Interlocked.Exchange. Only the first call would get back NOTCALLED in return while all subsequent calls would get back CALLED.

A better (and simpler) solution would be to use .Net's Lazy class which is perfect for initialization:

static Lazy<ExpensiveInstance> _lazy = new Lazy<ExpensiveInstance>(Initialization, LazyThreadSafetyMode.ExecutionAndPublication);

You can retrieve the result with _lazy.Value and query whether it was created with _lazy.IsValueCreated. The Initialization wouldn't run until it needs to and not more than once.

Peter Duniho · Answer 2 · 2014-12-19T22:03:48.583

0

I don't see why you are using Interlocked here instead of the much more readable and easily-understood lock statement. Personally, I would advise the latter.

Either way, the reason you are seeing one or more "exit" messages before a "set" message is due to thread scheduling. Even though the first thread to hit the Interlocked will always perform the "set" operation, that thread may be pre-empted before it gets a chance to do the operation, allowing some other thread to emit its "exit" message first.

Note that depending on your exact needs here, while using an if is the same as using a while loop, it's probable that neither accomplish what you want. I.e. the first thread to hit the if is going to set the value to the CALLED value, and so any other thread will just keep on going. If you're really trying to initialize something here, you probably want the other threads to wait for the thread that is actually executing the initialization code, so that all threads can proceed knowing the initialized state is valid.

I do think one way or the other it would be a good idea (i.e. less confusing to the user, and more importantly if the real code is more complicated than what you've shown, is more likely to produce correct results) if you synchronize the entire method.

Using Interlocked to accomplish that is much more complicated than the code you have now. It would involve a loop and at least one additional state value. But with a lock statement, it's simple and easy to read. It would look more like this:

const int NOTCALLED = 0;
const int CALLED = 1;
static int _state = NOTCALLED;
static readonly object _lock = new object();
//...
static void InterlockedCheck(string thread)
{
    lock (_lock)
    {
        Console.WriteLine("Enter thread [{0}], state [{1}]", thread, _state);

        if (_state == NOTCALLED)
        {
            Console.WriteLine("Setting state on T[{0}], state[{1}]", thread, _state);
            _state = CALLED;
        }

        Console.WriteLine("Exit from thread [{0}] state[{1}]", thread, _state);
    }
}

That way, the first thread to acquire the lock gets to execute all of its code before any other thread does, and in particular it ensures that the "set" operation in that first thread happens before the "exit" operation in any other thread.

edited Dec 19 '14 at 22:03

answered Dec 19 '14 at 16:32

Peter Duniho

68,759
7
102
136

You probably should be double checking that `lock` – i3arnon Dec 19 '14 at 19:52
I assume you mean "double-checking the `_state` value". And there'd be no point in doing so in this specific example. The point of the lock here is both to protect the handling of the `_state` field _and_ to ensure orderly execution of the "enter" and "exit" messages. A double-checked implementation would not be able to accomplish the latter. In any case, the performance improvement of a double-checked lock almost never is enough to justify the increased complexity of the code. – Peter Duniho Dec 19 '14 at 21:57
The "enter" and "exit" messages don't need to be inside the lock. There's no value in making these messages part of the critical section. Double checking is a known pattern, and it improves performance of the fast-path. Considering the OP is asking about `Interlocked` operations performance is probably an issue. If double checking is too complicated then `Lazy` (which is optimized for the fast-path) does that for you. – i3arnon Dec 20 '14 at 14:26
"There's no value in making these messages part of the critical section" -- the question _specifically_ notes the concern that the "exit" message occurs out of order with the expected initialization. Inasmuch as all of this code is obviously not the real code, that clearly means to me that it _does_ need to be part of the critical section in a correct solution. In any case, clarifications need to come from the OP, not other people. Double-checking is a known pattern, but also a known pitfall because it's usually implemented wrong _and often does not improve performance usefully_. – Peter Duniho Dec 20 '14 at 16:09
And `Lazy` works great for initialization of an instance of an object or value, but it's semantically awkward when the initialization involves multiple operations and no clear return value. The best way to perform this type of initialization would actually be in a constructor (static or instance, depending on need). Finally, the fact that the OP is asking about `Interlocked` means nothing; lots of people think they need lock-free code when in fact there's no evidence to support that belief at all. But thank you for your input. – Peter Duniho Dec 20 '14 at 16:13
thank you all for your input. Those that noticed were correct this is far from production code and was missing the appropriate read:lock:read:set. I accept that the lock {} would be easier to read, and potentially queue up the thread calls whilst initialization is carried out. I just don't like it. Seems heavy handed clumsy and far from elegant. If I can achieve the same through checking an integer value then that's what I would prefer. I forgot about Lazy and will look into that. Thankyou. – DubMan Dec 22 '14 at 13:32
FWIW: using `lock` (or `Lazy` for that matter) is no more "inelegant" than is using any of the other many features built into C# and the .NET frameworks, instead of re-implementing all of them yourself. If you take your philosophy to its logical conclusion, you would not be using .NET at all. – Peter Duniho Dec 22 '14 at 17:26

Interlocked.exchange as a lock

2 Answers2