Is it thread-safe to iterate over an immutable copy of Dictionary values if this copy was made under a lock?

Question

Is the below code thread-safe?

I need to call an async method on every service, therefore I cannot keep the foreach loop under the lock.

But would it be thread-safe to copy all the values from the _dictionary to an ImmutableList under the lock, exit the lock and then iterate over them as usual and call the async method?

public class Cache
{
    private readonly ReaderWriterLockSlim lockObject = new();

    private readonly IDictionary<string, Service> _dictionary = new Dictionary<string, Service>();

    public Service GetOrAdd(string key)
    {
        lockObject.EnterUpgradeableReadLock();
        try
        {
            if (_dictionary.TryGetValue(key, out var service))
            {
                return service;
            }
            lockObject.EnterWriteLock();
            try
            {
                var value = new Service();
                _dictionary.Add(key, value);
                return service;
            }
            finally
            {
                lockObject.ExitWriteLock();
            }
        }
        finally
        {
            lockObject.ExitUpgradeableReadLock();
        }
    }

    public async Task CallServices()
    {
        var services = GetServices();
        foreach (var service in services)
        {
            await service.DoStuffAsync();
        }
    }

    private ImmutableList<Service> GetServices()
    {
        lockObject.EnterReadLock();
        try
        {
            return _dictionary.Values.ToImmutableList();
        }
        finally
        {
            lockObject.ExitReadLock();
        }
    }
}

Replacing the Dictionary with the ConcurrentDictionary and removing the lock from the GetServices is one option, but I still have to keep the lock in GetOrAdd because in realilty I have 2 collections to maintain, not only the _dictionary. I was wondering if it was possible to use a normal Dictionary.

Answer 1

As far as I can tell, yes.

Since the copy of the dictionary is made while the lock is held there should be no risk that the dictionary is read and written to concurrently. And concurrent reads are safe:

A Dictionary<TKey,TValue> can support multiple readers concurrently, as long as the collection is not modified

The created copy is a local variable, and cannot be accessed by multiple threads, so using this is thread safe by default. There is not even any need to make it immutable, using a regular list would work just as well.

But this is specifically for the accessing the dictionary. .DoStuffAsync() could still be unsafe, but I assume that is out of scope.

Answer 2

No, it is not thread-safe. Just because the collection is immutable does not mean that the data IN the collection is immutable, and you do not have any locking to protect the individual records.

Consider the following modified cache code:

public class Cache
{
    private readonly ReaderWriterLockSlim lockObject = new();

    private readonly IDictionary<string, Service> _dictionary = new Dictionary<string, Service>();

    public Service GetOrAdd(string key)
    {
        lockObject.EnterUpgradeableReadLock();
        try
        {
            if (_dictionary.TryGetValue(key, out var service))
            {
                return service;
            }
            lockObject.EnterWriteLock();
            try
            {
                var value = new Service();
                _dictionary.Add(key, value);
                return service;
            }
            finally
            {
                lockObject.ExitWriteLock();
            }
        }
        finally
        {
            lockObject.ExitUpgradeableReadLock();
        }
    }

    public async Task CallServices()
    {
        var services = GetServices();
        foreach (var service in services)
        {
            await service.DoStuffAsync();
        }
    }

    private ImmutableList<Service> GetServices()
    {
        lockObject.EnterReadLock();
        try
        {
            return _dictionary.Values.ToImmutableList();
        }
        finally
        {
            lockObject.ExitReadLock();
        }
    }
    
    public void VerifyServices(int expectedCount)
    {
        var services = GetServices();
        foreach (var service in services)
        {
            if (service.Count != expectedCount)
            {
                throw new Exception($"Expected count: {expectedCount}. Actual count: {service.Count}");
            }
        }
    }
}

where Service modifies itself in DoStuffAsync:

public class Service
{
    public int Id { get; set; } = Guid.NewGuid().GetHashCode();
    public int Count { get; set; } = 1;
    
    public async Task DoStuffAsync()
    {
        Count++;
        await Task.Delay(10);
    }
}

If we test it with the following:

void Main()
{
    var cache = new Cache();
    
    for (int i = 0; i < 10; i++)
    {
        var key = i.ToString();
        cache.GetOrAdd(key);
    }
    
    var taskCount = 100;
    var tasks = new Task[taskCount];
    
    for (int i = 0; i < taskCount; i++)
    {
        tasks[i] = Task.Run(() => cache.CallServices());
    }
    
    Task.WhenAll(tasks).GetAwaiter().GetResult();
    
    cache.VerifyServices(taskCount);
}

an exception will be thrown during VerifyServices because the updates to the Service objects were done outside of the scope of a lock, even though the collection itself is immutable.

In conclusion, while the collection is immutable, there is nothing that guarantees that the contents themselves are safe from mutation. You'll have to determine if that caveat is sufficient for your needs.

Answer 3

Is the below code thread-safe?

So it depends what thread safety you are talking about - from dictionary standpoint of view it would be thread safe (actually I would argue that it is quite similar to want ConcurrentDictionary.Values does).

But from items returned by the collection it would not be thread safe and working with them will require extra synchronization (if needed).

Answer 4

What you are asking basically is whether enumerating an ImmutableList<T> is thread-safe. You are not concerned about the thread-safety of the T, but only about the thread-safety of enumerating the immutable collection. The answer is: it's perfectly safe. The immutable collections are thread-safe by design. Actually it is explicitly documented that the ImmutableList<T> collection is thread-safe:

This type is thread safe.

There are no exceptions. No ifs or whens. A thread can do whatever it wants with an ImmutableList<T>, and no other thread will ever be affected.

It should be noted that your GetServices method creates a new copy of the values of the _dictionary each time it is called, so the lists created by your code are not shared between threads, and so you wouldn't have any problems even if the ImmutableList<T> class was not thread-safe. I am pretty sure that if you change your code from return _dictionary.Values.ToImmutableList(); to return _dictionary.Values.ToList();, the correctness of your application will not be compromised to the slightest.

Answer 5

Trying to get the item from dictionary (dictionary.TryGetValue...) can be put outside of lock statement.

Lock only when item is not found and then, inside the lock ask again if it's not there, since many threads might find the first condition (dictionary.TryGetValue...) as false, and only if it's still false perform the insert.