7
\$\begingroup\$

This is a followup to a previous question.

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) == 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 = true;
            }
            catch
            {
                Dispose();
                _shouldDispose = false;
                throw;
            }
        }

        public void Dispose()
        {
            if (!_shouldDispose) return;
            if (_millisecondsTimeout < 0) _spinMutex.Unlock();
            else if (_locked) _spinMutex.Unlock();
        }
    }
}

And would be used like this:

static readonly SpinMutex MySpinMutex = new SpinMutex();

static void Process1()
{
    using (MySpinMutex.EnterScope()) { /* ... */ }
}

static void Process2()
{
    using (MySpinMutex.EnterScope()) { /* ... */ }
}

Instances of SpinMutex can be accessed from different Threads/Tasks safely.

\$\endgroup\$
  • 1
    \$\begingroup\$ Note that using based locks and Thread.Abort don't interact well. \$\endgroup\$ – CodesInChaos Dec 22 '14 at 16:59
  • \$\begingroup\$ The BCL contains a SpinLock struct, which might be a closer match for what you want to do than SpinWait+Interlocked.CompareExchange. \$\endgroup\$ – CodesInChaos Dec 22 '14 at 17:07
  • \$\begingroup\$ Thanks @CodesInChaos but SpinLock causes problems when we try to lock on the same thread twice; and when we use Tasks, it's most likely that we are-not-aware/can-not-control on which Thread we are. \$\endgroup\$ – Kaveh Shahbazian Dec 22 '14 at 17:13
2
\$\begingroup\$
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().

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.