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

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

Considering:

• Interlocked.CompareExchange generates a memory barrier,

• Intended to be used with very fast operations (like generating an

id),

• 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?

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:

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

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

StackExchange Code Review Q#74433, answer score: 3