views:

162

answers:

5

Is an atomic read guaranteed when you mix Interlocked operations with lock() (and other higher-level locks)?

I am interested in general behaviour when mixing locking mechanisms like this, and any differences between Int32 and Int64.

private Int64 count;
private object _myLock;

public Int64 Count 
{
  get 
  {
    lock(_myLock)
    {
      return count;
    }
  }
}

public void Increment
{
  Interlocked.Increment(ref count);
}
+2  A: 

The argument provided to the lock keyword must be an object based on a reference type. In your code, this is value type, this possibly means boxing, which makes lock statement useless.

Interlocked operation is atomic relatively to another Interlocked operation running in another thread, and applied to the same variable. There is no thread safety, if one thread uses Interlocked operation, and another changes the same variable by using another synchronization algorithm, or without synchronization.

Alex Farber
fixed code, sorry about that
Steve Townsend
+2  A: 

The answer to your question is no. The lock and Interlocked don't have anything to do with each other and they do not work together in the manner you are suggesting.

Also, not sure what is going on with your code. It does not compile. You can't lock on a value type. Also, Increment() takes a ref argument.

Bryan
fixed code, sorry about that
Steve Townsend
Yeah, that will at least compile now. But for sure, these two types of locks will not sync with each other. Also, you should never lock on this, but that is a totally different discussion.
Bryan
And now that I think about it, you don't need the lock on the property at all. Returning an int is an atomic operation. If this is all you have, you can remove the lock (this) and you will not have a problem.
Bryan
+2  A: 

First, you can't lock on value types, like int.

When you lock on a value type, it gets boxed into an object first. The problem is that it will be boxed every time and every time it will be a different "box". You will be locking on a different object every time, rendering the lock block useless.

That problem aside, let's say you you lock on a reference type, and you use the Interlocked class on another thread. There will be no synchronization between the threads. When you want synchronization you must use the same mechanism on both threads.

Martinho Fernandes
fixed code, sorry about that
Steve Townsend
+2  A: 

Is an atomic read guaranteed when you mix Interlocked operations with lock() (and other higher-level locks)?

An atomic read of int is always guaranteed regardless of locking of any kind. The C# specification states that reads of ints are always atomic.

I think that your actual question is different than the one you asked. Can you clarify the question?

Eric Lippert
@Eric - I intended to ask a more general question about mixing metaphors, edited accordingly
Steve Townsend
+7  A: 

Note: This answer (including the two edits already) was given before the question was changed to have a long count. For the current question instead of Thread.VolatileRead I would use Interlocked.Read, which also has volatile semantics and would also deal with the 64-bit read issue discussed here and introduced into the question

An atomic read is guaranteed without locking, because reads of properly-aligned values of 32-bit or less, which your count is, are guaranteed to be atomic.

This differs from a 64-bit value where if it started at -1, and was read while another thread was incrementing it, could result in the value read being -1 (happened before increment), 0 (happened after increment) or either of 4294967295 or -4294967296 (32 bits written to 0, other 32bits awaiting write).

The atomic increment of Interlocked.Increment means that the whole increment operation is atomic. Consider that increment is conceptually:

  1. Read the value.
  2. Add one to the value.
  3. Write the value.

Then if x is 54 and one thread tries to increment it while another tries to set it to 67, the two correct possible values are 67 (increment happens first, then is written over) or 68 (assignment happens first, then is incremented) but non-atomic increment can result in 55 (increment reads, assignment of 67 happens, increment writes).

A more common real case is x is 54 and one thread increments and another decrements. Here the only valid result is 54 (one up, then one down, or vice-versa), but if not atomic then possible results are 53, 54 and 55.

If you just want a count that is incremented atomically, the correct code is:

private int count;

public int Count 
{
  get 
  {
    return Thread.VolatileRead(byref count);
  }
}

public void Increment
{
  Interlocked.Increment(count);
}

If however you want to act upon that count, then it will need stronger locking. This is because the thread using the count can become out of date before its operation is finished. In this case you need to lock on everything that cares about the count and everything that changes it. Just how this need be done (and whether its even important to do at all) depends on more matters of your use-case than can be inferred from your question.

Edit: Oh, you may want to lock simply to force a memory barrier. You may also want to change Count's implementation to return Thread.VolatileRead(ref count); to make sure CPU caches are flushed if you are going to remove the lock. It depends on how important cache staleness is in this case. (Another alternative is to make count volatile, as then all reads and writes will be volatile. Note that this isn't needed for the Interlocked operations as they are always volatile.)

Edit 2: Indeed, you're so likely to want this volatile read, that I'm changing the answer above. It is possible you won't care about what it offers, but much less likely.

Jon Hanna
Shouldn't count be declared volatile in this case? Otherwise you would end up with register cashing issues, wouldn't you?
Martin Brown
@Martin. GMTA, was adding that as you commented. Again though, just what we want will depend on a few things. VolatileRead will serve what's in the question, but maybe `volatile` (as per your suggestion) would be better once other operations are added. It's worth adding for completeness that the `Interlocked` API has the correct memory barriers that volatility isn't needed with them (even says so in the documentation of the warning you get when you call them byref on a volatile field).
Jon Hanna
And kudos to the person asking http://stackoverflow.com/questions/3853678/why-lock-is-needed-to-implement-a-readonly-int-property/ since it was in thinking of my answer to that, that I realised it applied here too.
Jon Hanna