CODEWITHSUNDEEP
c#linqthread-safety
wal
wal

Reputation: 17749

linq deferred execution when using locks in methods that return IEnumerable

Consider a simple Registry class accessed by multiple threads:

public class Registry
{
    protected readonly Dictionary<int, string> _items = new Dictionary<int, string>();
    protected readonly object _lock = new object();

    public void Register(int id, string val)
    {
        lock(_lock)
        {
           _items.Add(id, val);
        }
    }

    public IEnumerable<int> Ids
    {
        get
        {
            lock (_lock)
            {
                return _items.Keys;
            }
        }
    }
}

and typical usage:

var ids1 = _registry.Ids;//execution deferred until line below
var ids2 = ids1.Select(p => p).ToArray();

This class is not thread safe as it's possible to receive System.InvalidOperationException

Collection was modified; enumeration operation may not execute.

when ids2 is assigned if another thread calls Register as the execution of _items.Keys is not performed under the lock!

This can be rectified by modifying Ids to return an IList:

public IList<int> Ids
    {
        get
        {
            lock (_lock)
            {
                return _items.Keys.ToList();
            }
        }
    }

but then you lose a lot of the 'goodness' of deferred execution, for example

var ids = _registry.Ids.First();  //much slower!

So,
1) In this particular case are there any thread-safe options that involve IEnumerable
2) What are some best practices when working with IEnumerable and locks ?

Upvotes: 13

Views: 1869

Answers (3)

vgru
vgru

Reputation: 51292

If you use yield return inside the property, then compiler will ensure that the lock is taken on first call to MoveNext(), and released when the enumerator is disposed.

I would avoid using this, however, because poorly implemented caller code might forget to call Dispose, creating a deadlock.

public IEnumerable<int> Ids     
{
    get      
    {
        lock (_lock)             
        {
            // compiler wraps this into a disposable class where 
            // Monitor.Enter is called inside `MoveNext`, 
            // and Monitor.Exit is called inside `Dispose`

            foreach (var item in _items.Keys)
               yield return item;
        }         
    }     
}

Upvotes: 1

Dr. Andrew Burnett-Thompson
Dr. Andrew Burnett-Thompson

Reputation: 21561

When your Ids property is accessed then the dictionary cannot be updated, however there is nothing to stop the Dictionary from being updated at the same time as LINQ deferred execution of the IEnumerator<int> it got from Ids.

Calling .ToArray() or .ToList() inside the Ids property and inside a lock will eliminate the threading issue here so long as the update of the dictionary is also locked. Without locking both update of the dictionary and ToArray(), it is still possible to cause a race condition as internally .ToArray() and .ToList() operate on IEnumerable.

In order to resolve this you need to either take the performance hit of ToArray inside a lock, plus lock your dictionary update, or you can create a custom IEnumerator<int> that itself is thread safe. Only through control of iteration (and locking at that point), or through locking around an array copy can you achieve this.

Some examples can be found below:

Upvotes: 9

jason
jason

Reputation: 241779

Just use ConcurrentDictionary<TKey, TValue>.

Note that ConcurrentDictionary<TKey, TValue>.GetEnumerator is thread-safe:

The enumerator returned from the dictionary is safe to use concurrently with reads and writes to the dictionary

Upvotes: 5

Related Questions