Bug with removing a list entry while in an IEnumerator

Unity Engine
,
1

Hello, I’m getting an InvalidOperationException: Collection was modified; enumeration operation may not execute. error when I try to remove an item from a list using list.Remove(item); inside a while loop inside an IEnumerator. Here is a cut down example of my code. Please ignore how many curly brackets there are, I haven’t counted them and there may be extras.

private IEnumerator Ticker()
        {
            while (true)
            {
                foreach (var s in stats)
                {
                    if (s.enabled)
                    {
                        foreach (var se in s.status)
                        {
                              s.status.Remove(se);
                        }
                        }
                    }
                }
                yield return new WaitForSecondsRealtime(7);
            }
        }

The specific problem is having Remove(se); in there, but I sorta need that. Any ideas how I can fix this? I suppose I could just cue it up to be removed in Update(), but that seems wasteful.

EDIT FOR ANYONE SEEKING THE ANSWER

You can add .ToList() to the list in the foreach however this is inefficient.

foreach (var item in list.ToList())
{
     list.Remove(item);
}

It is better to use a reverse for loop. Reverse because the list drops items backwards to fill space.

for (int i = s.status.Count - 1; i >= 0; i--)
{
    var se = s.status[i];
    // do stuff with se if needed
    s.status.RemoveAt(i);
}
2

Can’t modify the collection.

Make a new list record your entry data and then process the replacement of the original list with the new list after the for each has completed

3

Linked list can also be used. Be sure to pool the LinkedListNode when you remove a item otherwise you will strain the GC

4

Okay thanks guys

GC?

5

If you do not pool the LinkedListNode it will be garbage collected which can lead to spikes

6

You can do

foreach (var s in stats**.ToArray()**)

Or do it in the foreach loop below that. Should solve the problem immediately, but im not sure if its good practice outside of prototyping.

1 Like
7

It’s a o(n) operation plus allocation. Depends on use case if bad

1 Like
8

Thank you. What do you mean by LinkedListNode? Can you link to anything about using that in the way you describe?

9

The LinkedListNode is the element type in a linked list. When you add a new item to a linked list it’s a LinkedListNode being added. In same fashion a LinkedListNode is removed when removing from a linked list. Since LinkedListNode is a reference type it allocates on the heap.

10

Oh! Thank you!

11

To modify a list from a loop, you need to iterate the list with indexes instead of using an enumerator. The usual way is with a reverse for loop :

for (int i = s.status.Count - 1; i >= 0; i--)
{
    var se = s.status[i];
    // do stuff with se if needed
    s.status.RemoveAt(i);
}

Also note that if you’re removing all entries, you can (should) just call s.status.Clear() after the foreach loop instead of removing each element one by one.

1 Like
12

Thanks. I’ve not really seen a reverse loop before. Any reason I’d use that?

13

Otherwise you would miss an item since you advance to the next index

14

It’s mainly convenience. You can’t directly do a forward for loop because you would skip entries. To do a forward iteration while deleting some entries, you need to use a while loop :

int i = 0;
while (i < list.Count)
{
  if (someCondition)
    list.RemoveAt(i);
  else
    i++;
}
1 Like
15

Ah, okay, thanks.

16

By the way, I remembered I had this issue before and I’ve rediscovered the fix in my old code files. Just add .ToList() to the list in the foreach

foreach (var item in list.ToList())
{
     list.Remove(item);
}
17

Just be aware of the O(n) and allocation implication

18

Thanks, could you explain what you mean a little?

19

Why not just use List.clear()? That’s all that code above does, right? Even if that’s too fancy,
list = new List<whatever>() should do the same thing, but better.

20

While this technically work, it’s an incredibly inefficient way of doing this, and a bad practice in general.

Not only you’re instantiating an useless new list (which is slow and is increasing GC pressure), but using Remove(item) is causing a full iteration of reference comparisons on the list, which is also very slow. Plus, it’s not safe to use if your list has duplicate entries.

Learn the good habit of using a reverse for loop and RemoveAt(index). Better write efficient and safe code the first time, that will save you a lot of time and trouble in the long run.

And as @Owen-Reynolds mentionned, if you’re removing all entries every time, just do a List.Clear() after the foreach loop instead of removing entries one by one. That is the cleanest and most performant way.