2

I've been getting this exception out in the field. I don't understand how the collection can be modified while iterating. I copy everything to local variables at the start of the method.

  public void Flush() {
            var tempEntities = attachedEntities.Select(item => item).ToList();
            attachedEntities.Clear();

            var tempEntitiesToDelete = entitiesToDelete.Select(item => item).ToList();
            entitiesToDelete.Clear();

            foreach (var attachedEntity in tempEntities) {
                var isTransient = (bool)GetPrivateField(attachedEntity.GetType(), attachedEntity, "isTransient");
                if (isTransient)
                    db.Insert(attachedEntity);
                else
                    db.Update(attachedEntity);
            }

            foreach (var entity in tempEntitiesToDelete)
                db.Delete(entity);
        }

Stack Trace

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
  at System.Collections.Generic.List`1 Enumerator[Compass.Mobile.Core.DataAccess.IEntity].VerifyState () [0x00000] in <filename unknown>:0 
  at System.Collections.Generic.List`1 Enumerator[Compass.Mobile.Core.DataAccess.IEntity].MoveNext () [0x00000] in <filename unknown>:0 
  at System.Linq.Enumerable <CreateSelectIterator>c__Iterator1D`2[Compass.Mobile.Core.DataAccess.IEntity,Compass.Mobile.Core.DataAccess.IEntity].MoveNext () [0x00000] in <filename unknown>:0 
  at System.Collections.Generic.List`1[Compass.Mobile.Core.DataAccess.IEntity].AddEnumerable (IEnumerable`1 enumerable) [0x00000] in <filename unknown>:0 
  at System.Collections.Generic.List`1[Compass.Mobile.Core.DataAccess.IEntity]..ctor (IEnumerable`1 collection) [0x00000] in <filename unknown>:0 
  at System.Linq.Enumerable.ToList[TSource] (IEnumerable`1 source) [0x00000] in <filename unknown>:0 
  at Compass.Mobile.Core.DataAccess.Session.Flush () [0x00000] in <filename unknown>:0 
  at Compass.Mobile.Core.DataAccess.Session.Commit () [0x00000] in <filename unknown>:0 
  at Compass.Mobile.Core.Bootstrap.CommandBus.Flush () [0x00000] in <filename unknown>:0
Chris Kooken
  • 32,730
  • 15
  • 85
  • 123
  • `.Select(item => item)` is a nop, by the way. It won't do anything useful. – Dan Abramov Feb 08 '13 at 19:06
  • 1
    Usually this is a threading problem. Some other thread modifying either the attachedEntities or entitiesToDelete objects while ToList() is iterating them. – Hans Passant Feb 08 '13 at 19:10
  • @Chris, try removing it. I think it's worse than a nop in this case. – Dan Abramov Feb 08 '13 at 19:10
  • Since you are `ToList`ing the enumerable, there's actually no need to `foreach` - you can use a basic `for(int i=0; i < list.Count; i++)` construct...but as to why you're getting the exception, I'd be guessing, but I'd say your `db` methods somehow alter the passed-in `entity` object enough to trigger an enumerable changed? Edit: no, that doesn't make sense...I think I'm with @HansPassant - something else is poking around in your data - another thread? – JerKimball Feb 08 '13 at 19:11
  • 1
    AHH. I didn't even think about the ToList(). I was looking at the loop...but the stacktrace clearly shows the exception happening on "ToList()" – Chris Kooken Feb 08 '13 at 19:14
  • How did you generate attachedEntities, which is not in view here? – Brian Mains Feb 08 '13 at 19:29

2 Answers2

3

Replace this:

foreach (var entity in tempEntitiesToDelete)
     db.Delete(entity);

With:

for (var i = tempEntitiesToDelete.Count - 1; i >= 0; i--)
   db.Delete(tempEntitiesToDelete[i]);

I had this problem when I tried to delete while looping through; it was trying to modify the list of items. Therefore, looping backward fixed it for me.

Brian Mains
  • 50,520
  • 35
  • 148
  • 257
  • 2
    adding .ToList() or .ToArray() to the tempEntitiesToDelete in the foreach would work too. – John Kraft Feb 08 '13 at 19:15
  • If exception happened while enumerating `tempEntities`, there wouldn't be `System.Linq.Enumerable c__Iterator1D``2` in the stack trace, would there? It's hard to say without the line numbers of course. – Dan Abramov Feb 08 '13 at 19:20
2

Judging by the stack trace, it's not failing inside your foreach (or enumerating tempEntities which is a plain list) but inside one of ToList calls, when Select iterator's underlying list checks its state before moving on to next item.

This line in your stack trace makes me believe so:

at System.Linq.Enumerable <CreateSelectIterator>c__Iterator1D`2[Compass.Mobile.Core.DataAccess.IEntity,Compass.Mobile.Core.DataAccess.IEntity].MoveNext () [0x00000] in <filename unknown>:0

Your foreach loop variable is just a list, so it doesn't go through Select. This leads me to believe that either attachedEntities or entitiesToDelete change while you're doing Select over them:

/* Failing here... */
var tempEntities = attachedEntities.Select(item => item).ToList();
attachedEntities.Clear();

/* ...or here*/
var tempEntitiesToDelete = entitiesToDelete.Select(item => item).ToList();
entitiesToDelete.Clear();

/* ...but not here! */
foreach (var attachedEntity in tempEntities) {
   // ...
}

It could very well be a concurrency issue.

By the way, you really don't need Select (item => item), it would just be an identity projection.

Dan Abramov
  • 264,556
  • 84
  • 409
  • 511
  • Yes. I just realized this as well. So whats the best way to take a "Snapshot" of the lists and process those entities. There are other threads accessing this. – Chris Kooken Feb 08 '13 at 19:21
  • 1
    @ChrisKooken create a new list, then lock/otherwise synchronize access, iterate over the original, inserting your "to do" items into the new list. Then process the new list(s). – JerKimball Feb 08 '13 at 19:27
  • You shouldn't have threads accessing shared lists **at all**. This is really dangerous because [lists are not thread-safe](http://stackoverflow.com/questions/5020486/listt-thread-safety). If it's too hard to change current design, consider placing `lock`s around the code accessing them (but beware of deadlocks), or, better, use types from `System.Collections.Concurrent` namespace. – Dan Abramov Feb 08 '13 at 19:28
  • If i were to use a ConcurrentBag, do i need any locks anywhere? – Chris Kooken Feb 08 '13 at 19:32
  • @Chris: If you only access these two lists from other threads, using `ConcurrentBag`s instead of `List`s should make your code thread-safe. [Take a look at this article](http://www.codethinked.com/net-40-and-system_collections_concurrent_concurrentbag). If you're accessing shared instances of other classes from different threads, you'll need locks if some of these classes aren't thread safe. You can check thread safety of any .NET class in MSDN. – Dan Abramov Feb 08 '13 at 19:47