Lock Using "Interlocked" vs lock Statement ("Monitor")

Question

A followup question can be found here, based on this question and it's answer.

Considering:

1. Interlocked.CompareExchange generates a memory barrier,
2. Intended to be used with very fast operations (like generating an id),
3. SpinWait starts sleeping in milliseconds after 10 or so spins (and does some other smart stuff),

What are characteristics (pros & cons) of this code?

public class SpinMutex
{
 const long Locked = 1;
 const long Unlocked = 0;
 long _locked;
 public SpinMutex() { _locked = Unlocked; }
 public void Lock()
 {
 SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked);
 }
 public bool Lock(int millisecondsTimeout)
 {
 return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked, millisecondsTimeout);
 }
 public bool Lock(TimeSpan timeout)
 {
 return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked, timeout);
 }
 public void Unlock()
 {
 Interlocked.Exchange(ref _locked, Unlocked);
 }
}

And some extra points to consider:

• Does it help if SpinMutex be defined as struct instead of a class (I'm aware there would be hurdles with say readonly fields of struct type and the like)?
• Should Interlocked.CompareExchange be used in Unlock too, instead of Interlocked.Exchange to ensure generating of a memory barrier? Or Interlocked.Exchange generates a memory barrier itself? Is a memory barrier needed at all at Unlock?

Answer 1

First and foremost I think your implementation is broken: Interlocked.CompareExchange returns the original value at the location. So in your lock methods you should spin until the original value was Unlocked and not Locked.

As for your first question: Besides that "better" is a very vague term - no I don't think making it a struct is better for the simple reason that you have a mutable structure (it can change it's own state between locked and unlocked) and mutable structs are generally best to be avoided - the value semantics of it can trip you up pretty quick.

For your second question: As per MSDN all Interlocked methods make use of the appropriate barriers.

In general:

1. I would check that the lock was actually locked in Unlock (and throw if it wasn't) - unlocking an already unlocked lock indicates a potentially serious bug (lock/unlock should always be in matching pairs).

2. I'd consider implementing IDisposable and check that the lock is not locked when being disposed. Any object owning a lock instance should then dispose of it - this adds another check that the locking/unlocking is done correctly.

Answer 2

• I would not use a long for _locked. You don't need such a big variable. On some 32 bit architectures atomically exchanging a value larger than 32 bits may incur a significant performance hit.
• Rename your bool returning Lock methods to TryLock. A method called Lock should either succeed or throw an exception.