Threading and IEnumerable; "Collection Was Modified"-exception

Imagine that in the class below, one thread gets the IEnumerable-object and starts iteration over the elements. While in the middle of the iteration, another开发者_如何学Go thread comes along and add a new entry to the library_entries via the Add-method. Would a "Collection was modified"-exception be thrown in the iteration? Or would the lock prevent the adding of the element until the iteration is complete? Or neither?

Thanks!

public static class Library
{
    private static List<string> library_entries = new List<string>(1000000);

    public static void Add(string entry)
    {
        lock (library_entries)
            library_entries.Add(entry);
    }

    public static IEnumerable<string> GetEntries()
    {
        return library_entries.Where(entry => !string.IsNullOrEmpty(entry));
    }
}

No, you won't get the exception, you are using a Linq query. It is much worse, it will fail unpredictably. The most typical outcome is that the same item gets enumerated twice, although anything is possible when the List re-allocates its internal storage during the Add() call, including an IndexOutOfRangeException. Once a week, give or take.

The code that calls GetEntries() and consumes the enumerator must acquire the lock as well. Locking the Where() expression is not good enough. Unless you create a copy of the list.

locking wouldn't help at all, since the iteration doesn't use locking. I recommend rewriting the GetEntries() function to return a copy.

public static IEnumerable<string> GetEntries()
{
    lock(lockObj)
    {
        return library_entries.Where(entry => !string.IsNullOrEmpty(entry)).ToList();
    }
}

Note this returns a consistent snapshots. i.e. it won't return newly added objects when you're iterating.

And I prefer to lock on a private object whose only purpose is locking, but since the list is private it's no real problem, just a stylistic issue.

You could also write your own iterator like this:

int i=0;
bool MoveNext()
{
  lock(lockObj)
  {
      if(i<list.Count)
          return list[i];
      i++;
  }
}

If that's a good idea depends on your access patterns, lock contention, size of the list,... You might also want to use a Read-Write lock to avoid contention from many read accesses.

The static GetEntries method doesn't perform any lock on the static library_entries collection => it is not thread-safe and any concurrent calls on it from multiple threads might break. The fact that you have locked the Add method is fine but enumerating is not a thread safe operation so you must lock on it as well if you intend to call the GetEntries method concurrently. Also because this method returns an IEnumerable<T> it won't do anything on the actual list until you start enumerating which could be outside of the GetEntries method. So you could add a .ToList() call at the end of the LINQ chaining and thenlock the entire operation.

Yes, an exception would be thrown - you aren't locking on a common object. Also, a lock in the GetEntries method would be useless, as that call returns instantly. The locking would've to occur while iterating.