While updating a value in concurrent dictionary is better to lock dictionary or value

Question

I am performing two updates on a value I get from TryGet I would like to know that which of these is better?

Option 1: Locking only out value?

if (HubMemory.AppUsers.TryGetValue(ConID, out OnlineInfo onlineinfo))
{
    lock (onlineinfo)
    {
        onlineinfo.SessionRequestId = 0;
        onlineinfo.AudioSessionRequestId = 0;
        onlineinfo.VideoSessionRequestId = 0;
    }
}

Option 2: Locking whole dictionary?

if (HubMemory.AppUsers.TryGetValue(ConID, out OnlineInfo onlineinfo))
{
    lock (HubMemory.AppUsers)
    {
        onlineinfo.SessionRequestId = 0;
        onlineinfo.AudioSessionRequestId = 0;
        onlineinfo.VideoSessionRequestId = 0;
    }
}

Answer 1

I'm going to suggest something different.

Firstly, you should be storing immutable types in the dictionary to avoid a lot of threading issues. As it is, any code could modify the contents of any items in the dictionary just by retrieving an item from it and changing its properties.

Secondly, ConcurrentDictionary provides the TryUpdate() method to allow you to update values in the dictionary without having to implement explicit locking.

TryUpdate() requires three parameters: The key of the item to update, the updated item and the original item that you got from the dictionary and then updated.

TryUpdate() then checks that the original has NOT been updated by comparing the value currently in the dictionary with the original that you pass to it. Only if it is the SAME does it actually update it with the new value and return true. Otherwise it returns false without updating it.

This allows you to detect and respond appropriately to cases where some other thread has changed the value of the item you're updating while you were updating it. You can either ignore this (in which case, first change gets priority) or try again until you succeed (in which case, last change gets priority). What you do depend on your situation.

Note that this requires that your type implements IEquatable<T>, since that is used by the ConcurrentDictionary to compare values.

Here's a sample console app that demonstrates this:

using System;
using System.Collections.Concurrent;
using System.Threading;
using System.Threading.Tasks;

namespace Demo
{
    sealed class Test: IEquatable<Test>
    {
        public Test(int value1, int value2, int value3)
        {
            Value1 = value1;
            Value2 = value2;
            Value3 = value3;
        }

        public Test(Test other) // Copy ctor.
        {
            Value1 = other.Value1;
            Value2 = other.Value2;
            Value3 = other.Value3;
        }

        public int Value1 { get; }
        public int Value2 { get; }
        public int Value3 { get; }

        #region IEquatable<Test> implementation (generated using Resharper)

        public bool Equals(Test other)
        {
            if (other is null)
                return false;

            if (ReferenceEquals(this, other))
                return true;

            return Value1 == other.Value1 && Value2 == other.Value2 && Value2 == other.Value3;
        }

        public override bool Equals(object obj)
        {
            return ReferenceEquals(this, obj) || obj is Test other && Equals(other);
        }

        public override int GetHashCode()
        {
            unchecked
            {
                return (Value1 * 397) ^ Value2;
            }
        }

        public static bool operator ==(Test left, Test right)
        {
            return Equals(left, right);
        }

        public static bool operator !=(Test left, Test right)
        {
            return !Equals(left, right);
        }

        #endregion
    }

    static class Program
    {
        static void Main()
        {
            var dict = new ConcurrentDictionary<int, Test>();

            dict.TryAdd(0, new Test(1000, 2000, 3000));
            dict.TryAdd(1, new Test(4000, 5000, 6000));
            dict.TryAdd(2, new Test(7000, 8000, 9000));

            Parallel.Invoke(() => update(dict), () => update(dict));
        }

        static void update(ConcurrentDictionary<int, Test> dict)
        {
            for (int i = 0; i < 100000; ++i)
            {
                for (int attempt = 0 ;; ++attempt)
                {
                    var original  = dict[0];
                    var modified  = new Test(original.Value1 + 1, original.Value2 + 1, original.Value3 + 1);
                    var updatedOk = dict.TryUpdate(1, modified, original);

                    if (updatedOk) // Updated OK so don't try again.
                        break;     // In some cases you might not care, so you would never try again.

                    Console.WriteLine($"dict.TryUpdate() returned false in iteration {i} attempt {attempt} on thread {Thread.CurrentThread.ManagedThreadId}");
                }
            }
        }
    }
}

There's a lot of boilerplate code there to support the IEquatable<T> implementation and also to support the immutability.

Fortunately, C# 9 has introduced the record type which makes immutable types much easier to implement. Here's the same sample console app that uses a record instead. Note that record types are immutable and also implement IEquality<T> for you:

using System;
using System.Collections.Concurrent;
using System.Threading;
using System.Threading.Tasks;

namespace System.Runtime.CompilerServices // Remove this if compiling with .Net 5
{                                         // This is to allow earlier versions of .Net to use records.
    class IsExternalInit {}
}

namespace Demo
{
    record Test(int Value1, int Value2, int Value3);

    static class Program
    {
        static void Main()
        {
            var dict = new ConcurrentDictionary<int, Test>();

            dict.TryAdd(0, new Test(1000, 2000, 3000));
            dict.TryAdd(1, new Test(4000, 5000, 6000));
            dict.TryAdd(2, new Test(7000, 8000, 9000));

            Parallel.Invoke(() => update(dict), () => update(dict));
        }

        static void update(ConcurrentDictionary<int, Test> dict)
        {
            for (int i = 0; i < 100000; ++i)
            {
                for (int attempt = 0 ;; ++attempt)
                {
                    var original  = dict[0];

                    var modified  = original with
                    {
                        Value1 = original.Value1 + 1,
                        Value2 = original.Value2 + 1,
                        Value3 = original.Value3 + 1
                    };

                    var updatedOk = dict.TryUpdate(1, modified, original);

                    if (updatedOk) // Updated OK so don't try again.
                        break;     // In some cases you might not care, so you would never try again.

                    Console.WriteLine($"dict.TryUpdate() returned false in iteration {i} attempt {attempt} on thread {Thread.CurrentThread.ManagedThreadId}");
                }
            }
        }
    }
}

Note how much shorter record Test is compared to class Test, even though it provides the same functionality. (Also note that I added class IsExternalInit to allow records to be used with .Net versions prior to .Net 5. If you're using .Net 5, you don't need that.)

Finally, note that you don't need to make your class immutable. The code I posted for the first example will work perfectly well if your class is mutable; it just won't stop other code from breaking things.

---

Addendum 1:

You may look at the output and wonder why so many retry attempts are made when the TryUpdate() fails. You might expect it to only need to retry a few times (depending on how many threads are concurrently attempting to modify the data). The answer to this is simply that the Console.WriteLine() takes so long that it's much more likely that some other thread changed the value in the dictionary again while we were writing to the console.

We can change the code slightly to only print the number of attempts OUTSIDE the loop like so (modifying the second example):

static void update(ConcurrentDictionary<int, Test> dict)
{
    for (int i = 0; i < 100000; ++i)
    {
        int attempt = 0;
        
        while (true)
        {
            var original  = dict[1];

            var modified  = original with
            {
                Value1 = original.Value1 + 1,
                Value2 = original.Value2 + 1,
                Value3 = original.Value3 + 1
            };

            var updatedOk = dict.TryUpdate(1, modified, original);

            if (updatedOk) // Updated OK so don't try again.
                break;     // In some cases you might not care, so you would never try again.

            ++attempt;
        }

        if (attempt > 0)
            Console.WriteLine($"dict.TryUpdate() took {attempt} retries in iteration {i} on thread {Thread.CurrentThread.ManagedThreadId}");
    }
}

With this change, we see that the number of retry attempts drops significantly. This shows the importance of minimising the amount of time spent in code between TryUpdate() attempts.

---

Addendum 2:

As noted by Theodor Zoulias below, you could also use ConcurrentDictionary<TKey,TValue>.AddOrUpdate(), as the example below shows. This is probably a better approach, but it is slightly harder to understand:

static void update(ConcurrentDictionary<int, Test> dict)
{
    for (int i = 0; i < 100000; ++i)
    {
        int attempt = 0;
        
        dict.AddOrUpdate(
            1,                        // Key to update.
            key => new Test(1, 2, 3), // Create new element; won't actually be called for this example.
            (key, existing) =>        // Update existing element. Key not needed for this example.
            {
                ++attempt;

                return existing with
                {
                    Value1 = existing.Value1 + 1,
                    Value2 = existing.Value2 + 1,
                    Value3 = existing.Value3 + 1
                };
            }
        );

        if (attempt > 1)
            Console.WriteLine($"dict.TryUpdate() took {attempt-1} retries in iteration {i} on thread {Thread.CurrentThread.ManagedThreadId}");
    }
}

Answer 2

If you just need to lock the dictionary value, for instance to make sure the 3 values are set at the same time. Then it doesn't really matter what reference type you lock over, just as long as it is a reference type, it's the same instance, and everything else that needs to read or modify those values are also locked on the same instance.

You can read more on how the Microsoft CLR implementation deals with locking and how and why locks work with a reference types here

Why Do Locks Require Instances In C#?

If you are trying to have internal consistency with the dictionary and the value, that's to say, if you are trying to protect not only the internal consistency of the dictionary and the setting and reading of object in the dictionary. Then the your lock is not appropriate at all.

You would need to place a lock around the entire statement (including the TryGetValue) and every other place where you add to the dictionary or read/modify the value. Once again, the object you lock over is not important, just as long as it's consistent.

_{Note 1 : it is normal to use a dedicated instance to lock over (i.e. some instantiated object) either statically or an instance member depending on your needs, as there is less chance of you shooting yourself in the foot.}

_{Note 2 : there are a lot more ways that can implement thread safety here, depending on your needs, if you are happy with stale values, whether you need every ounce of performance, and if you have a degree in minimal lock coding and how much effort and innate safety you want to bake in. And that is entirely up to you and your solution.}

Answer 3

The first option (locking on the entry of the dictionary) is more efficient because it is unlikely to create significant contention for the lock. For this to happen, two threads should try to update the same entry at the same time. The second option (locking on the entire dictionary) is quite possible to create contention under heavy usage, because two threads will be synchronized even if they try to update different entries concurrently.

The first option is also more in the spirit of using a ConcurrentDictionary<K,V> in the first place. If you are going to lock on the entire dictionary, you might as well use a normal Dictionary<K,V> instead. Regarding this dilemma, you may find this question interesting: When should I use ConcurrentDictionary and Dictionary?