I'm looping through a List of elements with foreach, like this:

foreach (Type name in aList) {
   name.doSomething();
}

However, in an another thread I am calling something like

aList.Remove(Element);

During runtime, this causes an InvalidOperationException: Collection was modified; enumeration operation may not execute. What is the best way to handle this (I would perfer it to be rather simple even at the cost of performance)?

Thanks!

• c#
• multithreading
• list
• thread-safety

Thread A:

lock (aList) {
  foreach (Type name in aList) {
     name.doSomething();
  }
}

Thread B:

lock (aList) {
  aList.Remove(Element);
}

This ofcourse is really bad for performance.

What is the best way to handle this (I would perfer it to be rather simple even at the cost of performance)?

Fundamentally: don't try to modify a non-thread-safe collection from multiple threads without locking. The fact that you're iterating is mostly irrelevant here - it just helped you find it quicker. It would have been unsafe for two threads to both be calling Remove at the same time.

Either use a thread-safe collection such as ConcurrentBag or make sure that only one thread does anything with the collection at a time.

Method #1:

The simplest and least efficient method is to create a critical section for the readers and writers.

// Writer
lock (aList)
{
  aList.Remove(item);
}

// Reader
lock (aList)
{
  foreach (T name in aList)
  {
    name.doSomething();
  }
}

Method #2:

This is similar to method #1, but instead of holding the lock for the entire duration of the foreach loop you would copy the collection first and then iterate over the copy.

// Writer
lock (aList)
{
  aList.Remove(item);
}

// Reader
List<T> copy;
lock (aList)
{
  copy = new List<T>(aList);
}
foreach (T name in copy)
{
  name.doSomething();
}

Method #3:

It all depends on your specific situation, but the way I normally deal with this is to keep the master reference to the collection immutable. That way you never have to synchronize access on the reader side. The writer side of things needs a lock. The reader side needs nothing which means the readers stay highly concurrent. The only thing you need to do is mark the aList reference as volatile.

// Variable declaration
object lockref = new object();
volatile List<T> aList = new List<T>();

// Writer
lock (lockref)
{
  var copy = new List<T>(aList);
  copy.Remove(item);
  aList = copy;
}

// Reader
List<T> local = aList;
foreach (T name in local)
{
  name.doSomething();
}

If you have more than one reader, then try a Reader-Writer Lock (.Net 3.5+), Slim: http://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim.aspx

If you just have one reader, the just a lock on the list itself or a private object (but don't lock on the type itself), as shown in Eugen Rieck's answer.

if you just want to avoid the Exception use

foreach (Type name in aList.ToArray()) 
{ name.doSomething(); }

be aware the doSomething() is executed also in the case the element was removed in the other thread

I can't specifically tell from your question, but (it looks like) you are performing an action on each item and then removing it. You might want to look in to BlockingCollection<T>, which has a method calling GetConsumingEnumerable() to see if it's a good fit for you. Here's a small sample.

void SomeMethod()
{
    BlockingCollection<int> col = new BlockingCollection<int>();

    Task.StartNew( () => { 

        for (int j = 0; j < 50; j++)
        {
            col.Add(j);
        }

        col.CompleteAdding(); 

     });

    foreach (var item in col.GetConsumingEnumerable())
    {
       //item is removed from the collection here, do something
       Console.WriteLine(item);
    }
}