C# .NET : is "lock" required around ImmutableList reference copy?

Question

A debate formed about a lock statement in our team - I would appreciate some external feedback about this.

The code part in question is:

lock (IndexLock)
{
    currentIndex = Index;
}

You can see part of the class below:

public class BuilderService
{
    private ImmutableList<Model> Index { get; set; } = ImmutableList<Model>.Empty;
    private object IndexLock { get; } = new();

    public Add(Model item)
    {
        lock (IndexLock)
        {
           Index = Index.Add(item);
        }
    }

    public IEnumerable<Model> GetFilterLinesExcluding()
    {
        var filters = new List<Model>();
        ImmutableList<FilterModel> currentIndex;
    
        lock (IndexLock)
        {
            currentIndex = Index;
        }
    
        foreach (var filter in currentIndex)
        {
            ...
        }        
    }
}

Without going into the details, the debate is around phenomenons like:

• memory visibility
• memory barriers
• stale values

Does it make sense to have a lock around currentIndex = Index?

Threads:

• GetFilterLinesExcluding method accessed by multiple threads.
• Add method can be called at any time.

I will provide more information if required.

Thank you in advance!

Answer 1

Does it make sense to have a lock around currentIndex = Index?

Yes. Using the lock statement is the easiest way to enforce thread-safety, without having to do a lot of studying. The lock ensures that only one thread at a time can interact with the shared state, making multithreading relatively easy, provided that you are careful and disciplined. The lock statement also adds implicitly the appropriate memory barriers when the execution flow enters and exits the protected section, so you don't have to think about them. Your code in its current state is safe, correct, and reasonably efficient. It makes sense to keep your code as is.

A debate formed about a lock statement in our team.

I guess that someone in your team suggested to remove the lock around the currentIndex = Index; line. Their suggestion is wrong. Their argument most likely is that the ImmutableList<T> is immutable, so it's not possible for another thread to mutate it while the ImmutableList<T> is enumerated. It's a rational argument, but lock-free multithreading is anything but rational. You might not be aware that the C# compiler, as well as the .NET Jitter and the CPU processor, are all allowed to reorder the instructions of a program, providing that the reordering does not affect the execution on a single thread. In other words the compiler/Jitter/processor assumes that your program is single-threaded, unless you tell it otherwise by adding memory barriers implicitly or explicitly. So if you omit the lock around the currentIndex = Index;, instructions that succeed this line might be executed before this line. In this particular case I cannot see anything after this line that can be moved and placed before the line, and cause problems.

There is another source of danger though. The same reordering of instructions that can happen around the currentIndex = Index;, can also happen inside the Add method around the Index = Index.Add(item); line. Instructions related to the Index.Add can be moved and placed after the Index assignment. In other words the Index might be pointing, for a brief period of time, to a partially initialized ImmutableList<T> instance. This window of danger closes when the thread that executes the Index = Index.Add(item); exits the lock block, because there is an implicit memory barrier placed there. So by omitting the lock around the currentIndex = Index;, you are risking that some threads will try to enumerate ImmutableList<T> instances that are partially initialized, resulting in undefined behavior. The lock around the currentIndex = Index; prevents this, by ensuring that this window of danger will never be open. No thread will be able to enumerate a ImmutableList<T> instance before the memory barrier in the Add has been issued.

As a side note, an alternative to locking that might be suitable for your scenario, and is also non-contentious and more efficient, is the volatile keyword:

private volatile ImmutableList<Model> _index;

// Not thread-safe. Only one thread is supposed to call it.
public Add(Model item)
{
   _index = _index.Add(item);
}

// Thread-safe. Multiple threads can call it.
public IEnumerable<Model> GetFilterLinesExcluding()
{
    List<Model> filters = new();

    foreach (var filter in _index)
    {
        ...
    }        
}

The volatile adds the appropriate barriers whenever the _index is read or written, preventing the scenario of an ImmutableList<Model> being viewed in a partially initialized state.

In case you wanted to make the Add thread-safe as well, without using the lock statement, you could use the ImmutableInterlocked.Update API. This would prevent race conditions that could cause some Model items to be lost.

Although this use of the volatile is safe and correct, you might still prefer to avoid it because as a keyword is rather mystified. If you see that your coworkers aren't comfortable with it, it might be preferable to stick with the lock, instead of spending time to educate them about the correct use of this advanced keyword.