0

I have to lock objects by their keys (UIDs), do some staff and unlock. And I want to keep my dictionary clear. So here I use nested lock. The first is locking the whole dictionary just to safely perform RemoveLock(uid) at the end. And the second is locking an object to do stuff. My question is should i fear deadlock in this code?

        private static readonly ConcurrentDictionary<Guid, object> _lockDict = new ConcurrentDictionary<Guid, object>();

        private static object _listLock;

        public static void RunWithLock(Guid uid, Action body)
        {
            lock (_listLock)
            {
                var obj = GetLock(uid);

                lock (obj)
                {
                    body();
                }

                RemoveLock(uid);
            }
        }

        public static void RemoveLock(Guid uid)
        {
            _lockDict.TryRemove(uid, out _);
        }

        public static object GetLock(Guid uid)
        {
            return _lockDict.GetOrAdd(uid, s => new object());
        }

Tried to run it, didn't get any deadlocks. But thousands of objects processing every minute can make it deadlock.

Pavel Grishin
  • 1
  • 4
  • Why is the `_listLock` field named so? It seems that it protects a dictionary, not a list. – Theodor Zoulias Jun 08 '23 at 21:48
  • Since you have locked the ConcurentDictionary and then you have locked the object if there is not other impelemntations using these lock you should be ok but you can always set a timeout for a lock to release it automatically i Highly recomend you to read this https://stackoverflow.com/questions/15361810/how-to-find-out-deadlock-and-prevent-it-in-c-sharp#:~:text=To%20prevent%20deadlocks%2C%20you%20have,in%20a%20well%2Ddefined%20order. – Khashayar Pakkhesal Jun 08 '23 at 21:49
  • Have you looked at this question? [Asynchronous locking based on a key](https://stackoverflow.com/questions/31138179/asynchronous-locking-based-on-a-key). It features far superior ways for keeping the dictionary clean. – Theodor Zoulias Jun 08 '23 at 21:50

1 Answers1

1

My question is should I fear deadlock in this code?

No, there is no risk of deadlock. The locks are always acquired in the same order, first the _listLock and then one of the objects in the dictionary, so the deadlock condition that is described by Dijkstra in his famous Dining philosophers problem cannot occur.

That said, protecting a ConcurrentDictionary<K,V> with a locker object defeats the purpose of using a ConcurrentDictionary<K,V> in the first place. You get nothing compared to using a normal Dictionary<K,V>, apart from extra overhead and memory allocations. Also calling the body while holding the _listLock is terrible. It means that all the body invocations will be serialized, defeating the purpose of using any kind of dictionary, concurrent or not.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • You are right. I found the answer here https://codereview.stackexchange.com/questions/275685/c-lock-on-string-value This solution really helps processing locks one by one and in the same time keeping the dictionary clear. – Pavel Grishin Jun 09 '23 at 12:35
  • @Pavel Grishin I think that the [linked implementation](https://codereview.stackexchange.com/questions/275685/c-lock-on-string-value/275690#275690) (`StringLock`) is buggy. I posted a comment under the answer explaining why. – Theodor Zoulias Jun 09 '23 at 12:59