The exception is likely the result of having the collection changed in the middle of an iteration via IEnumerator. There are few techniques you can use to maintain thread-safety. I will present them in order of difficultly.
Lock Everything
This is by far the easiest and most trivial method for getting access to the data structure thread-safe. This pattern works well when the number of read and write operations are equally matched.
LinkedList<object> collection = new LinkedList<object>();
void Write()
{
lock (collection)
{
collection.AddLast(GetSomeObject());
}
}
void Read()
{
lock (collection)
{
foreach (object item in collection)
{
DoSomething(item);
}
}
}
Copy-Read Pattern
This is a slightly more complex pattern. You will notice that a copy of the data structure is made prior to reading it. This pattern works well when the number of read operations are few compared to the number of writes and the penalty of the copy is relatively small.
LinkedList<object> collection = new LinkedList<object>();
void Write()
{
lock (collection)
{
collection.AddLast(GetSomeObject());
}
}
void Read()
{
LinkedList<object> copy;
lock (collection)
{
copy = new LinkedList<object>(collection);
}
foreach (object item in copy)
{
DoSomething(item);
}
}
Copy-Modify-Swap Pattern
And finally we have the most complex and error prone pattern. I actually do not recommend using this pattern unless you really know what you are doing. Any deviation from what I have below could lead to problems. It is easy to mess this one up. In fact, I have inadvertently screwed this one up as well in the past. You will notice that a copy of the data structure is made prior to all modifications. The copy is then modified and finally the original reference is swapped out with the new instance. Basically we are always treating collection as if it were immutable. This pattern works well when the number of write operations are few compared to the number of reads and the penalty of the copy is relatively small.
object lockobj = new object();
volatile LinkedList<object> collection = new LinkedList<object>();
void Write()
{
lock (lockobj)
{
var copy = new LinkedList<object>(collection);
copy.AddLast(GetSomeObject());
collection = copy;
}
}
void Read()
{
LinkedList<object> local = collection;
foreach (object item in local)
{
DoSomething(item);
}
}
Update:
So I posed two questions in the comment section:
- Why
lock(lockobj) instead of lock(collection) on the write side?
- Why
local = collection on the read side?
Concerning the first question consider how the C# compiler will expand the lock.
void Write()
{
bool acquired = false;
object temp = lockobj;
try
{
Monitor.Enter(temp, ref acquired);
var copy = new LinkedList<object>(collection);
copy.AddLast(GetSomeObject());
collection = copy;
}
finally
{
if (acquired) Monitor.Exit(temp);
}
}
Now hopefully it is easier to see what can go wrong if we used collection as the lock expression.
- Thread A executes
object temp = collection.
- Thread B executes
collection = copy.
- Thread C executes
object temp = collection.
- Thread A acquires the lock with the original reference.
- Thread C acquires the lock with the new reference.
Clearly this would be disasterous! Writes would get lost since the critical section is entered more than once.
Now the second question was a little tricky. You do not necessarily have to do this with the code I posted above. But, that is because I used the collection only once. Now consider the following code.
void Read()
{
object x = collection.Last;
// The collection may get swapped out right here.
object y = collection.Last;
if (x != y)
{
Console.WriteLine("It could happen!");
}
}
The problem here is that collection could get swapped out at anytime. This would be an incredibly difficult bug to find. This is why I always extract a local reference on the read side when doing this pattern. That ensure we are using the same collection on each read operation.
Again, because problems like these are so subtle I do not recommend using this pattern unless you really need to.
