Is the following thread safe?

Question

I have the following code and wonder whether it is thread safe. I only lock when I add or remove items from the collection but do not lock when I iterate over the collection. Locking while iterating would severely impact performance because the collection potentially contains hundreds of thousands of items. Any advice what to do to make this thread safe?

Thanks

public class Item
{
    public string DataPoint { get; private set; }

    public Item(string dataPoint)
    {
        DataPoint = dataPoint;
    }
}

public class Test
{
    private List<Item> _items; 
    private readonly object myListLock = new object();

    public Test()
    {
        _items = new List<Item>();
    }

    public void Subscribe(Item item)
    {
        lock (myListLock)
        {
            if (!_items.Contains(item))
            {
                _items.Add(item);
            }
        }
    }

    public void Unsubscribe(Item item)
    {
        lock (myListLock)
        {
            if (_items.Contains(item))
            {
                _items.Remove(item);
            }
        }
    }

    public void Iterate()
    {
        foreach (var item in _items)
        {
            var dp = item.DataPoint;
        }
    }

}

EDIT

I was curious and again profiled performance between an iteration that is not locked vs iterating inside the lock on the myListLock and the performance overhead of locking the iteration over 10 million items was actually quite minimal.

Answer 1

No, it isn't thread safe, because the collection could be modified while you look inside it... What you could do:

Item[] items; 

lock (myListLock)
{
    items = _items.ToArray();
}

foreach (var item in items)
{
    var dp = item.DataPoint;
}

so you duplicate the collection inside a lock before cycling on it. This clearly will use memory (because you have to duplicate the List<>) (ConcurrentBag<>.GetEnumerator() does nearly exactly this)

Note that this works only if Item is thread safe (for example because it is immutable)

Answer 2

No it isn't. Note that all classes documented on MSDN has a section on Thread Safety (near the end): https://msdn.microsoft.com/en-us/library/6sh2ey19%28v=vs.110%29.aspx

The documentation for GetEnumerator has some more notes: https://msdn.microsoft.com/en-us/library/b0yss765%28v=vs.110%29.aspx

The key point is that iteration is not itself thread-safe. Even if each individual iterated read from the collection is thread safe, a consistent iteration often breaks down if the collection is modified. You could get problems such as reading the same element twice or skipping some elements, even if the collection itself is never in an inconsistent state.

Btw, your Unsubscribe() is doing TWO linear searches of the list, which is probably not what you want. You shouldn't need to call Contains() before Remove().

Answer 3

In theory your code is not threadsafe.

---

In the background foreachperforms a normal for loop and if you add an item from a distinct thread while foreachiterates through your list, an item might be left out. Also if you remove an item (from a distinct thread), you might get an AV exception or - even worse - gibberish data.

If you want your code to be threadsafe, you have two choices:

1. You can clone your list (I usually use the .ToArray() method for that purpose). It will lead to doubling the list in the memory with all its cost and the result might not be up-to-date for in-situ editions, or...
2. You can put your entire iteration in a locked block, which will result in blocking other threads accessing the array while you are performing a long-running operation.