ReadOnlyCollections and Threads - Is this code safe?

Question

I have a heavily threaded application with a ReadOnlyCollection as follows:

internal static ReadOnlyCollection<DistributorBackpressure44> DistributorBackpressure44Cache
{
    get
    {
        return _distributorBackpressure44;
    }
    set
    {
        _distributorBackpressure44 = value;
    }
}

I have one place in the app where this collection is replaced (always on a separate thread) and it looks like this:

    CicApplication.DistributorBackpressure44Cache = new ReadOnlyCollection<DistributorBackpressure44>(someQueryResults.ToList());

I have many places in the code where this collection is accessed, usually via Linq queries, in many different threads. The code often looks something like this:

foreach (DistributorBackpressure44 distributorBackpressure44 in CicApplication.DistributorBackpressure44Cache.Where(row => row.Coater == coater && row.CoaterTime >= targetTime).ToList())
{
 ...
 ...
}

I assume what I'm doing is thread-safe, without the need to do any locking? What I'm not sure about is what happens with the query above if it occurs at the exact same time the collection is getting replaced in a different thread?

score 5 · Accepted Answer · edited May 23 '17 at 12:04

5

Reference assignments are atomic, so yes, it is thread safe. But only as long as you don't rely on the data to be ready to be read exactly the moment after it is written. This is because of caching, you might want to throw in a volatile to prevent that.

See also reference assignment is atomic so why is Interlocked.Exchange(ref Object, Object) needed?.

edited May 23 '17 at 12:04

Community

1
1

answered Nov 29 '12 at 18:34

Sebastian Graf

3,602
3
27
38

I'm not sure exactly what you mean. However, if you mean that some readers of the data might have an 'old' copy and some readers might have a 'new' copy, that would be acceptable. – Randy Minder Nov 29 '12 at 18:38

score 2 · Answer 2 · answered Nov 29 '12 at 18:50

It is pretty unlikely. At an entirely unpredictable moment in time, after the thread assigns the property, other threads are going to see the new collection. They'll read a stale value before that. Which might be null or might be another collection with entirely different content.

The randomness is what will get you in trouble. Do note that this has nothing to do with whether or not the collection is ReadOnly. Maybe it is okay that the threads use a stale value, but that isn't very common. Most of all you didn't mention it was okay so you might not yet have considered the consequences. You will need to think this through. There isn't anything you can do about that yourself in the property getter and setter, the threads are going to have to negotiate between themselves. This is what makes threading hard and makes it very important that you thoroughly analyzes the places in the code where mutable data is shared. Like this property.

score 0 · Answer 3 · answered May 27 '14 at 14:29

It sounds like it won't be a problem in your case, but if the underlying collection that the ReadOnlyCollection is wrapping changes, you could run into problems.

For example, the following code chunk will throw an InvalidOperationException with the message "Collection was modified; enumeration operation may not execute" since the underlying List<int> has an item removed from it while it is being enumerated over on another thread.

var numbers = new List<int>() {1,2,3,4,5};
var readOnly = new ReadOnlyCollection<int>(numbers);

ThreadPool.QueueUserWorkItem(x => {
    foreach (int number in readOnly)
    {
        Console.WriteLine(number);
        Thread.Sleep(300);
    }
});

Thread.Sleep(150);
numbers.Remove(2);

ReadOnlyCollections and Threads - Is this code safe?

3 Answers3