1
\$\begingroup\$

The following code is greatly inspired on this question and this answer. That answer works quite well for a specific case, but I was looking for a solution that:

  1. could be used more generic (so I can put this in a base library and never have to think about it anymore).
  2. got rid of the Find() and replace it with a O1 dictionary lookup. To be honest I doubt if this would have any performance effect, but it was tickling my OCDs.

I've tried to test it with running about 10 threads and superficially it seems to work. But as concurrency is hard to test and reason about - am I missing something? Any other obvious upgrades?

public class LockDictionary<TKey>
{
    object _RootLock = new object();
    Dictionary<TKey,ItemLock> _KeyToLock = new Dictionary<TKey,ItemLock>();


    public IDisposable Lock(TKey key)
    {
        ItemLock item_lock;

        lock  ( _RootLock )
        {
            item_lock = GetOrCreate(key);
            item_lock._Counter++;
        }

        Monitor.Enter(item_lock);

        return new LockCounter(this, key);
    }


    private ItemLock GetOrCreate(TKey key)
    {
        if ( !_KeyToLock.TryGetValue(key, out var item_lock) )
        {
            item_lock = new ItemLock();
            _KeyToLock.Add( key, item_lock );
        }

        return item_lock;
    }


    private void ReleaseLock(TKey key)
    {
        lock (_RootLock)
        {
            var item_lock = _KeyToLock[key];
            item_lock._Counter--;

            if ( item_lock._Counter == 0 )
                _KeyToLock.Remove(key);

            Monitor.Exit(item_lock);
        }
    }


    private class ItemLock
    {
        public object   _ItemLock   = new object();
        public int      _Counter    = 0;
    }


    private class LockCounter : IDisposable
    {
        LockDictionary<TKey> _Parent;
        TKey _Key;


        public LockCounter(LockDictionary<TKey> parent, TKey key)
        {
            _Parent     = parent;
            _Key        = key;
        }


        public void Dispose()
        {
            _Parent.ReleaseLock(_Key);
        }
    }
}

Update

To clarify; I'm looking for an implementation for the following:

// minimal definition boilerplate
// possibility of using other types as key, like string, long or a struct
static LockDictionary<int> _LockDictionary = new LockDictionary<int>();

// minimal boilerplate for having a separate lock per "groupId"
using ( _LockDictionary.Lock(groupId) )
{
     // can only be run once concurrently per unique groupId
}
\$\endgroup\$
  • \$\begingroup\$ Why you're not locking Monitor.Enter(item_lock) with your _RootLock but locking Monitor.Exit(item_lock);? Seems a bit inconsistent to me \$\endgroup\$ – Bohdan Stupak Nov 5 at 9:25
  • \$\begingroup\$ Good point - I guess I can move the Monitor.Exit outside the _RootLock. \$\endgroup\$ – Dirk Boer Nov 5 at 9:30
  • \$\begingroup\$ If it fits your needs I'd rewrite this to use a ConditionalWeakTable, it's probably faster, slimmer and already well tested. Also note that if Lock() fails anywhere you might have orphans in your dictionary (and acquired but unreleased locks and/or inconsistencies between _Counter and calls to Monitor.Enter()). In short...hmmm...instead of trying to have a complex locking mechanism which should be extremely well reviewed (and with good chances to take it wrong), what are you trying to achieve? \$\endgroup\$ – Adriano Repetti Nov 5 at 10:33
  • \$\begingroup\$ Hi @AdrianoRepetti, I've updated the question with the "interface" I'd like to fill. I'm not familiar with ConditionalWeakTable but if that makes it easy to be wrapped that would be preferred of course. \$\endgroup\$ – Dirk Boer Nov 5 at 10:42
1
+50
\$\begingroup\$
  1. ItemLock._ItemLock is not used anywhere. Also, as a side note, I'm not a fan of using _NamesLikeThis for public items
  2. Using a single lock may not be the best idea performance-wise (though heavily scenario-dependent). Consider using CuncurrentDictionary (which of course brings all sorts of new propblems, but this is how concurrency works). Also, you may consider using a ReadWriteLockSlim, it might be more performant in some scenarios
  3. ReleaseLock is not safe if not matched by a Lock call. Which might happen for example in case of a ThreadAbort. An ugly, but widespread way of dealing with it is placing the critical code to a finally block. This article from the SQL Server people might also be of interest: https://docs.microsoft.com/en-us/dotnet/framework/performance/reliability-best-practices
\$\endgroup\$
  • \$\begingroup\$ Hi @vvoton, thanks for your answer. Sorry I broke it with some refactoring during copy pasting - it should now work again. \$\endgroup\$ – Dirk Boer Nov 9 at 7:33
  • \$\begingroup\$ What would the fix be for point 3? \$\endgroup\$ – Dirk Boer Nov 9 at 7:34
  • 1
    \$\begingroup\$ @DirkBoer,the usual (though quite ugly) hack is to put the critical code inside a finally section. There is a nice article on reliablity from the SQL server team here docs.microsoft.com/en-us/dotnet/framework/performance/… \$\endgroup\$ – vvotan Nov 9 at 8:30
  • 1
    \$\begingroup\$ @Mast, I've updated the answer \$\endgroup\$ – vvotan Nov 9 at 12:36
  • 1
    \$\begingroup\$ @DirkBoer Usually Dispose does not do anything critical. If it fails, there is always a GC to pick things up. You should only care if you class has some critical resource like a mutex of a file handle and your process absolutely needs to accommodate for this rather low-probability scenario, like for example an SQL server. And there are classes like SafeHandle that do this more or less automagically. Also, .NET core does not support ThreadAborts, which are the most dangerous of the async exceptions so this becomes less of an issue \$\endgroup\$ – vvotan Nov 11 at 15:34

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.