Identify InvalidOperationException "Collection was modified; enumeration operation may not execute."

Question

I've got a good old InvalidOperationException being thrown with the standard message

Collection was modified; enumeration operation may not execute.

The problem is, the enumerator isn't modifying itself, for example:

private TRoute _copyRoute(TRoute route)
{
    TRoute tempRoute = new TRoute();
    tempRoute.Initialize(route.Resource);

    foreach (TVisit visit in route)
    {
       tempRoute.Add(visit);
    }
    tempRoute.EndLocation = route.EndLocation;
    return tempRoute;
}

My code is multi-threaded (circa 12-15 threads for this example) and each thread is supposed to be working on its own deep clone of a route. Obviously something is going wrong somewhere, but, my question is how do I track this down with so many threads? Reducing the number significantly stops the problem manifesting itself.

In this case my route instance is an IList so I can play around with adding things to the interface. Underneath it has it's own List implementation.

EDIT

Just to add, I could ToArray() or ToList() this and maybe ignore the problem here but I don't really want to do that, I want to locate the cause. For example:

If I change it to the following:

private TRoute _copyRoute(TRoute route)
{
    TRoute tempRoute = new TRoute();
    tempRoute.Initialize(route.Resource);

    foreach (TVisit visit in route.ToList())
    {
       tempRoute.Add(visit);
    }
    tempRoute.EndLocation = route.EndLocation;
    return tempRoute;
}

Then I fail on this Assert, because a chance has occurred just before ToList()... I need to try and find out where that change is occuring

TRoute tempRoute1 = CopyRoute(route1);
TRoute tempRoute2 = CopyRoute(route2);
Debug.Assert(tempRoute1.Count == route1.Count);

Answer 1

Here's something you can use to wrap your IList<T> - it checks that it's on the right thread on each write operation. Of course, it would still be unsafe to be iterating over this on one thread while writing on another, but I assume that's not the problem. (You could always call CheckThread on all operations, not just the writing ones.)

using System;
using System.Collections;
using System.Collections.Generic;
using System.Threading;

class ThreadAffineList<T> : IList<T>
{
    private readonly Thread expectedThread;
    private readonly IList<T> list;

    public ThreadAffineList(IList<T> list)
    {
        this.list = list;
        this.expectedThread = Thread.CurrentThread;
    }

    private void CheckThread()
    {
        if (Thread.CurrentThread != expectedThread)
        {
            throw new InvalidOperationException("Incorrect thread");
        }
    }

    // Modification methods: delegate after checking thread
    public T this[int index]
    {
        get { return list[index]; }
        set
        {
            CheckThread();
            list[index] = value;
        }
    }

    public void Add(T item)
    {
        CheckThread();
        list.Add(item);
    }

    public void Clear()
    {
        CheckThread();
        list.Clear();
    }

    public void Insert(int index, T item)
    {
        CheckThread();
        list.Insert(index, item);
    }

    public bool Remove(T item)
    {
        CheckThread();
        return list.Remove(item);
    }

    public void RemoveAt(int index)
    {
        CheckThread();
        list.RemoveAt(index);
    }

    // Read-only members
    public int Count { get { return list.Count; } }
    public bool IsReadOnly { get { return list.IsReadOnly; } }

    public IEnumerator<T> GetEnumerator()
    {
        return list.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public bool Contains(T item)
    {
        return list.Contains(item);
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        list.CopyTo(array, arrayIndex);
    }

    public int IndexOf(T item)
    {
        return list.IndexOf(item);
    }
}

Answer 2

Assuming you're in control of Add(TVisit)/Remove(TVisit) of TRoute underlying collection:

1. Extend your TRoute.IEnumerator<TVisit> GetEnumerator() to set an AutoResetEvent or Mutex

2. Extend your Add(TVisit)/Remove(TVisit) methods to wait for event/mutex with zero timeout

   if(!autoReseEvent.WaitOne(0)) throw new MyException();
   

3. Catch MyExpcetion and you'll get the stack trace and change origin.

UPDATE: The issue with this approach is when to release event/mutex. You'll probably have to decorate your enumerator with a new class like below:

public IEnumerator<TVisit> GetEnumerator()
{
    IEnumerator<TVisit> originEnum = // get it somehow from underlying collection
    IEnumerator<TVisit> evenlope = new DisposableEvenlope<TVisit>(originEnum);
    evenlope.Disposed += new EventHandler(/* do your magic and reset event/mutex here */);
    return evenlope;
}

And the evelope itself:

public class DisposableEvenlope<T> : IEnumerator<T>
{
    private IEnumerator<T> _privateEnum;

    public event System.EventHandler Disposed;

    public DisposableEvenlope(IEnumerator<T> privateEnum)
    {
        _privateEnum = privateEnum;
    }

    public T Current
    {
        get { return _privateEnum.Current; }
    }

    public void Dispose()
    {
        Disposed(this, new System.EventArgs());
    }

    object IEnumerator.Current
    {
        get { return _privateEnum.Current; }
    }

    public bool MoveNext()
    {
        return _privateEnum.MoveNext();
    }

    public void Reset()
    {
        _privateEnum.Reset();
    }
}

Answer 3

The problem is obviously not in the code you have written as you are not modifying the collection while enumerating.

Somehow one of two things is happening:

1. Your route is somehow not a deep clone and some thread is modyfying the same collection you are passing to the pasted logic (that should be easy to track is it is a coding issue somewhere, not some nasty race condition).
2. Your exception is being raised precisely in the cloning logic and not in the code you have pasted.

Try implementing some locking mechanism while deep cloning your collection and see if that solves the issue.

Answer 4

Since you are aware that mutiple thread can touch route, prevent any touch on route with a lock(), and always in a lock() convert route.ToArray() and then use that array for loop. This since you probably feel some performance drawbacks if you lock() the entire loop. To catch who is really touching the collection, you can derive it and track the thread id in a local member variable in the add/remove elements.