17

I haven't used Queues<T> to any real degree before, so I might be missing something obvious. I'm trying to iterate through a Queue<EnemyUserControl> like this (every frame):

foreach (var e in qEnemy)
{
     //enemy AI code
}

When an enemy dies, the enemy user control raises an event I've subscribed to and I do this (the first enemy in the queue is removed by design):

void Enemy_Killed(object sender, EventArgs e)
{      
     qEnemy.Dequeue();

     //Added TrimExcess to check if the error was caused by NULL values in the Queue (it wasn't :))
     qEnemy.TrimExcess();
}

However, after the Dequeue method is called, I get an InvalidOperationException on the foreach loop. When I use Peek instead, there are no errors so it has to do something with the changing of the Queue itself since Dequeue removes the object. My initial guess is that it's complaining that I'm modifying a collection which is being iterated by the Enumerator, but the dequeuing is being performed outside the loop?

Any ideas what could be causing this issue?

Thanks

Alex Aza
  • 76,499
  • 26
  • 155
  • 134
keyboardP
  • 68,824
  • 13
  • 156
  • 205

6 Answers6

31

I know this is an old post but what about the following :

var queue = new Queue<int>();
queue.Enqueue(1);
queue.Enqueue(2);

while (queue.Count > 0)
{
  var val = queue.Dequeue();
}

Cheers

tinonetic
  • 7,751
  • 11
  • 54
  • 79
DarkUrse
  • 2,084
  • 3
  • 25
  • 33
  • 11
    I'd recommend changing this around slightly to a while/do instead of do/while, so that you're performing the .Count check before attempting the first .Dequeue(), in case the queue is empty. – DaveD Nov 04 '15 at 15:10
  • 2
    An otherwise great answer! Just edited it in agreement with @DaveD's concern – tinonetic Mar 14 '19 at 06:13
22

You are modifying queue inside of foreach loop. This is what causes the exception.
Simplified code to demonstrate the issue:

var queue = new Queue<int>();
queue.Enqueue(1);
queue.Enqueue(2);

foreach (var i in queue)
{
    queue.Dequeue();
}

Possible solution is to add ToList(), like this:

foreach (var i in queue.ToList())
{
    queue.Dequeue();
}
Alex Aza
  • 76,499
  • 26
  • 155
  • 134
  • D'oh, bit of a facepalm moment. One of the methods in the AI code calls a `Movement` method which, in turn, raises the killed event (I thought it was raised by some code outside the loop), so the dequeue is performed within the loop. The `ToList()` method works perfectly. Thanks! – keyboardP Jun 04 '11 at 01:46
5

Old post but thought I would provide a better answer:

var queue = new Queue<int>();
queue.Enqueue(1);
queue.Enqueue(2);


while (queue?.Count > 0))
{
  var val = queue.Dequeue();
}

As DarkUrse's original answer used a do/while and that would cause an exception if the queue is empty when trying to de-queue on the empty queue, also added a protection against a null queue

JohnChris
  • 1,360
  • 15
  • 29
  • 1
    queue.Count > 0 doesn't throw any exception is the queue is empty. Only if it's null. And queue.Any() will also throw NullReferenceException in this case. So what do you mean? – JohnIdlewood Nov 13 '19 at 11:16
  • @AlexeyKhrenov so if you look at DarkUrse's answer's edit history, you will see it used to be a do/while, and if the queue was empty, an exception would be thrown when trying to dequeue. You are somewhat right though as my answer should contain the previous answer I was fixing, as someones edit now invalidates my answer... I will edit it and also add a protection against a null queue – JohnChris Nov 15 '19 at 10:45
  • 2
    @AlexeyKhrenov my answer has now been updated to better explain its original intent... and also added the null check that you raised. I hope that clarifies everything, thanks – JohnChris Nov 15 '19 at 10:51
  • The new `queue?.` syntactic sugar available in .NET makes this a much nicer implementation than the alternative of wrapping the `while` loop in a `null` check. – Jonathan E. Landrum Aug 10 '21 at 15:05
1

This is typical behavior of enumerators. Most enumerators are designed to function correctly only if the underlying collection remains static. If the collection is changed while enumerating a collection then the next call to MoveNext, which is injected for you by the foreach block, will generate this exception.

The Dequeue operation obviously changes the collection and that is what is causing the problem. The workaround is to add each item you want removed from the target collection into a second collection. After the loop is complete you can then cycle through the second collection and remove from the target.

However, this might be a bit awkward, at the very least, since the Dequeue operation only removes the next item. You may have to switch to a different collection type which allows arbitrary removals.

If you want to stick with a Queue then you will be forced to dequeue each item and conditionally re-queue those items which should not be removed. You will still need the second collection to keep track of the items that are okay to omit from the re-queueing.

Brian Gideon
  • 47,849
  • 13
  • 107
  • 150
0

You can't remove elements from a collection while iterating over them.

The best solution I've found is to use a "List<> toDelete" and add whatever you want to remove to that list. Once the foreach loop ends, you can remove the elements from the target collection using the references in the toDelete list like so:

foreach (var e in toDelete)
    target.Remove(e);
toDelete.Clear();

Now since this is a Queue, you might simply be able to count the number of times you want to Dequeue in an integer and use a simple for loop to execute them later (I do not have that much experience with queues in this regard).

June Rhodes
  • 3,047
  • 4
  • 23
  • 29
  • You can simply iterate over the queue and clear it. In that case the effect is the same as using a List, so there is really no point using a Queue for this (if you don't need to dequeue individually). – arni Dec 11 '14 at 09:29
0

It doesn't matter where you are modifying the collection. If a collection is modified while you are enumerating its members, you get exception. You can use locks and make sure the collection is not modified when iterrating or if you are using .NET 4.0 replace Queue with ConcurrentQueue.

Xaqron
  • 29,931
  • 42
  • 140
  • 205