Why doesn't a foreach loop work in certain cases?

Question

I was using a foreach loop to go through a list of data to process (removing said data once processed--this was inside a lock). This method caused an ArgumentException now and then.

Catching it would have been expensive so I tried tracking down the issue but I couldn't figure it out.

I have since switched to a for loop and the problem seems to have went away. Can someone explain what happened? Even with the exception message I don't quite understand what took place behind the scenes.

Why is the for loop apparently working? Did I set up the foreach loop wrong or what?

This is pretty much how my loops were set up:

foreach (string data in new List<string>(Foo.Requests))
{
    // Process the data.

    lock (Foo.Requests)
    {
        Foo.Requests.Remove(data);
    }
}

and

for (int i = 0; i < Foo.Requests.Count; i++)
{
    string data = Foo.Requests[i];

    // Process the data.

    lock (Foo.Requests)
    {
        Foo.Requests.Remove(data);
    }
}

EDIT: The for* loop is in a while setup like so:

while (running)
{
    // [...]
}

EDIT: Added more information about the exception as requested.

System.ArgumentException: Destination array was not long enough. Check destIndex and length, and the array's lower bounds
  at System.Array.Copy (System.Array sourceArray, Int32 sourceIndex, System.Array destinationArray, Int32 destinationIndex, Int32 length) [0x00000] 
  at System.Collections.Generic.List`1[System.String].CopyTo (System.String[] array, Int32 arrayIndex) [0x00000] 
  at System.Collections.Generic.List`1[System.String].AddCollection (ICollection`1 collection) [0x00000] 
  at System.Collections.Generic.List`1[System.String]..ctor (IEnumerable`1 collection) [0x00000]

EDIT: The reason for the locking is that there is another thread adding data. Also, eventually, more than one thread will be processing data (so if the entire setup is wrong, please advise).

EDIT: It was hard to pick a good answer.

I found Eric Lippert's comment deserving but he didn't really answer (up-voted his comment anyhow).

Pavel Minaev, Joel Coehoorn and Thorarin all gave answers I liked and up-voted. Thorarin also took an extra 20 minutes to write some helpful code.

I which I could accept all 3 and have it split the reputation but alas.

Pavel Minaev is the next deserving so he gets the credit.

Thanks for the help good people. :)

Answer 1

Your problem is that the constructor of List<T> that creates a new list from IEnumerable (which is what you call) isn't thread-safe with respect to its argument. What happens is that while this:

 new List<string>(Foo.Requests)

is executing, another thread changes Foo.Requests. You'll have to lock it for the duration of that call.

[EDIT]

As pointed out by Eric, another problem List<T> isn't guaranteed safe for readers to read while another thread is changing it, either. I.e. concurrent readers are okay, but concurrent reader and writer are not. And while you lock your writes against each other, you don't lock your reads against your writes.

Answer 2

Your locking scheme is broken. You need to lock Foo.Requests() for the entire duration of the loop, not just when removing an item. Otherwise the item might become invalid in the middle of your "process the data" operation and enumeration might change in between moving from item to item. And that assumes you don't need to insert the collection during this interval as well. If that's the case, you really need to re-factor to use a proper producer/consumer queue.