Modifying list from another thread while iterating (C#)

Go To StackoverFlow.com

8

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!

2012-04-04 19:17
by Henrik Karlsson


8

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.

2012-04-04 19:19
by Eugen Rieck
Thanks, works like a charm :) Let's hope I won't get any performance problems by using that. - Henrik Karlsson 2012-04-04 19:33


10

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.

2012-04-04 19:19
by Jon Skeet
Didn't think about that, but now that you said that it makes sense. Thanks, I will probably make use of that later, but for just this case lock works fine - Henrik Karlsson 2012-04-04 19:38


9

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();
}
2012-04-04 20:01
by Brian Gideon
I like the variety of solutions here, but it should be noted in the first two examples that aList should be an internal object of the class otherwise a deadlock could occur - Derek W 2015-05-07 01:31
@DerekW: Yes, that is a good practice and well noted. In reality it is unlikely to actually cause a deadlock because the typical scenarios for such occurrences (nested locks, etc.) are not in play here. So unless another piece of code totally abuses aList the worst that would likely happen is increased lock contention. I think the whole lock(this) debate is a bit overblown and leans a little too far towards the paranoia side than some will admit. Again, it is still good practice to avoid such idioms though - Brian Gideon 2015-05-07 02:53
I should also point out while I'm thinking about it that #3 needs to be followed exactly as presented. Any deviation (like deciding that the assignment to local can be ommitted) will likely lead to random and spectacular failures. It might even rip a whole in spacetime for all I know - Brian Gideon 2015-05-07 02:55


1

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.

2012-04-04 19:36
by user1198042


0

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

2012-04-04 19:31
by NoMe
Unfortunately ToArray and Remove could race possibly resulting in a different kind of exception. It is a good thought though! Refer to my answer to see how to get this to work correctly - Brian Gideon 2012-04-04 20:09
Oh...and welcome to stackoverflow : - Brian Gideon 2012-04-04 20:41


0

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);
    }
}
2012-04-04 19:33
by Bryan Crosby
Ads