Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Below is attempt to catch potential deadlocks at runtime in C#. Basically this class allows user to acquire locks in a pre-defined order only. I ran some basic tests and it appears to work. Please review and let me know if you find something wrong with implementation.

public class OrderedLock
{
    [ThreadStatic]
    static HashSet<int> mAcquiredLocks;
    static ConcurrentDictionary<int, object> mCreatedLocks = new ConcurrentDictionary<int, object>();

    private int mId;
    private ThreadLocal<int> mRefCount = new ThreadLocal<int>(false);
    private ReaderWriterLockSlim mLock;

    public OrderedLock(int id)
    {
        if (!mCreatedLocks.TryAdd(id, null)) throw new ApplicationException("Duplicate identifier detected");

        mId = id;
        mRefCount.Value = 0;
        mLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    }

    public void AcquireReadLock()
    {
        CheckLockOrder();
        mLock.EnterReadLock();
    }

    public void AcquireWriteLock()
    {
        CheckLockOrder();
        mLock.EnterWriteLock();
    }

    public void ReleaseReadLock()
    {
        mRefCount.Value -= 1;
        mLock.ExitReadLock();
        if (mRefCount.Value == 0) mAcquiredLocks.Remove(mId);
    }

    public void ReleaseWriteLock()
    {
        mRefCount.Value -= 1;
        mLock.ExitWriteLock();
        if (mRefCount.Value == 0) mAcquiredLocks.Remove(mId);
    }

    private void CheckLockOrder()
    {
        if (mAcquiredLocks == null) mAcquiredLocks = new HashSet<int>();
        if (!mAcquiredLocks.Contains(mId))
        {
            if (mAcquiredLocks.Count > 0 && mAcquiredLocks.Max() > mId)
                throw new ApplicationException("Invalid order of locking detected");
            else
                mAcquiredLocks.Add(mId);
        }

        mRefCount.Value += 1;
    }
}
share|improve this question
add comment (requires an account with 50 reputation)

1 Answer

up vote 1 down vote accepted

I haven't extensively tested it, but there are a few things I've noted:

  • The class has two members which hold on to IDisposable resources. Therefore, the class must implement IDisposable as well and Dispose() of those members.
  • Many of the class members are assigned at construction time only. Mark them as readonly to signify intent and to potentially allow the runtime to perform some optimizations.
  • Current naming conventions favor camelCasing over Hungarian Notation for class members.
  • += 1 and -= 1 can be simplified to ++ and --, respectively.
  • ApplicationException has been out of favor for quite a long time.
  • Identify your class members with the this keyword.
  • Forgot to put this bullet point in on the first cut: replace .Count() > 0 with .Any(). This is idiomatic to what you want, rather than how to do it. In other words, the implementation of the former could very well iterate over the entire collection to get the count, while the latter may just check for the existence of one in its collection.
  • Another omission: I replaced the declaration of HashSet<int> with ISet<int> since coding to interfaces allows for more flexibility in changing implementations later on.

All that being said, here's a first-cut rework:

public class OrderedLock : IDisposable
{
    private static readonly ConcurrentDictionary<int, object> createdLocks = new ConcurrentDictionary<int, object>();
    [ThreadStatic]
    private static ISet<int> acquiredLocks;

    private readonly ThreadLocal<int> refCount = new ThreadLocal<int>(false);
    private readonly ReaderWriterLockSlim locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    private readonly int id;

    /// <exception cref="InvalidOperationException">Duplicate identifier detected</exception>
    public OrderedLock(int id)
    {
        if (!createdLocks.TryAdd(id, null))
        {
            throw new InvalidOperationException("Duplicate identifier detected");
        }

        this.id = id;
        this.refCount.Value = 0;
    }

    public void AcquireReadLock()
    {
        this.CheckLockOrder();
        this.locker.EnterReadLock();
    }

    public void AcquireWriteLock()
    {
        this.CheckLockOrder();
        this.locker.EnterWriteLock();
    }

    public void ReleaseReadLock()
    {
        this.refCount.Value--;
        this.locker.ExitReadLock();
        if (this.refCount.Value == 0)
        {
            acquiredLocks.Remove(this.id);
        }
    }

    public void ReleaseWriteLock()
    {
        this.refCount.Value--;
        this.locker.ExitWriteLock();
        if (this.refCount.Value == 0)
        {
            acquiredLocks.Remove(this.id);
        }
    }

    public void Dispose()
    {
        while (this.locker.IsWriteLockHeld)
        {
            this.ReleaseWriteLock();
        }

        while (this.locker.IsReadLockHeld)
        {
            ReleaseReadLock();
        }

        this.locker.Dispose();
        this.refCount.Dispose();
        GC.SuppressFinalize(this);
    }

    /// <exception cref="InvalidOperationException">Invalid order of locking detected</exception>
    private void CheckLockOrder()
    {
        if (acquiredLocks == null)
        {
            acquiredLocks = new HashSet<int>();
        }

        if (!acquiredLocks.Contains(this.id))
        {
            if (acquiredLocks.Any() && acquiredLocks.Max() > this.id)
            {
                throw new InvalidOperationException("Invalid order of locking detected");
            }

            acquiredLocks.Add(this.id);
        }

        this.refCount.Value++;
    }
}
share|improve this answer
Thank you for your valuable comments Jesse. – pavy bez Mar 18 at 22:48
add comment (requires an account with 50 reputation)

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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