Lock Using “Interlocked” vs lock Statement (“Monitor”) (Followup)

This is a followup to a previous question.

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) == Unlocked);
}

/public/
bool Lock(int millisecondsTimeout)
{
return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Unlocked, millisecondsTimeout);
}

/public/
bool Lock(TimeSpan timeout)
{
return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Unlocked, timeout);
}

/public/
void Unlock()
{
Interlocked.CompareExchange(ref _locked, Unlocked, Locked);
}

public IDisposable EnterScope()
{
return new Scope(this);
}

public IDisposable EnterScope(int millisecondsTimeout)
{
return new Scope(this, millisecondsTimeout);
}

class Scope : IDisposable
{
SpinMutex _spinMutex;
int _millisecondsTimeout;
bool _locked;
bool _shouldDispose;

internal Scope(SpinMutex spinMutex, int millisecondsTimeout = -1)
{
_spinMutex = spinMutex;
_millisecondsTimeout = millisecondsTimeout < -1 ? -1 : millisecondsTimeout;
_locked = false;
_shouldDispose = true;

try
{
if (_millisecondsTimeout < 0) _spinMutex.Lock();
else if (!_spinMutex.Lock(_millisecondsTimeout)) throw new TimeoutException();
else _locked

void Unlock()
{
    Interlocked.CompareExchange(ref _locked, Unlocked, Locked);
}

So, if you try to unlock when the lock is not locked, nothing happens? That sounds like a recipe for well-hidden bugs. Instead, unlocking when not locked should throw an exception.

_millisecondsTimeout = millisecondsTimeout < -1 ? -1 : millisecondsTimeout;

Why are you making sure that negative values are changed to -1, when later on you just check whether the timeout is negative?

Anyway, the approach taken by .Net methods like SpinUntil() is that negative values other than -1 throw, so I think you should do that too to be consistent.

if (_millisecondsTimeout < 0) _spinMutex.Lock();
else if (!_spinMutex.Lock(_millisecondsTimeout)) throw new TimeoutException();
else _locked = true;

I don't like having the three branches on the same level here, because logically, they're not. I think this way makes more sense:

if (_millisecondsTimeout < 0)
{
    _spinMutex.Lock();
}
else
{
    if (!_spinMutex.Lock(_millisecondsTimeout)) throw new TimeoutException();
    else _locked = true;
}

Also, what's the purpose of the first if at all. Removing it will work the same:

if (!_spinMutex.Lock(_millisecondsTimeout)) throw new TimeoutException();
else _locked = true;

catch
{
    Dispose();
    _shouldDispose = false;
    throw;
}

Calling Dispose() here makes sense only if the lock gets locked (so TimeoutException wasn't thrown) but then an exception was thrown. But most of the time, such exceptions (e.g. ThreadAbortException) are catastrophic and the whole process is going to go down, so you don't need to care about them much.

_shouldDispose = false;

I don't understand this. If the constructor throws an exception, the caller never gets their hands on the created instance, so they won't be able to call Dispose(). This means that _shouldDispose = false here doesn't make much sense.

On the other hand, calling Dispose() more than once should work (“If an object's Dispose method is called more than once, the object must ignore all calls after the first one.”), but won't, so adding _shouldDispose = false directly into Dispose() would make sense.

I think that _locked should be set always when the lock is locked, not just under some circumstances. That would make understanding your code easier and it would also simplify code in both in the constructor and in Dispose().

StackExchange Code Review Q#74475, answer score: 2