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!
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();
}
aList
should be an internal object of the class otherwise a deadlock could occur - Derek W 2015-05-07 01:31
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
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
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
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
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);
}
}