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

I know it's not what you asked for, but just for the sake of my own sanity, does the following represent the intention of your code:

private object _locker = new object();

// ...

lock (_locker) {
    Foo.Requests.Clear();
}

Answer 2

After seeing your exception; it looks to me that Foo.Requests is being changed while the shallow copy is being constructed. Change it to something like this:

List<string> requests;

lock (Foo.Requests)
{
    requests = new List<string>(Foo.Requests);
}

foreach (string data in requests)
{
    // Process the data.

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

---

Not the question, but...

That being said, I somewhat doubt the above is what you want either. If new requests are coming in during processing, they will not have been processed when your foreach loop terminates. Since I was bored, here's something along the lines that I think you're trying to achieve:

class RequestProcessingThread
{
    // Used to signal this thread when there is new work to be done
    private AutoResetEvent _processingNeeded = new AutoResetEvent(true);

    // Used for request to terminate processing
    private ManualResetEvent _stopProcessing = new ManualResetEvent(false);

    // Signalled when thread has stopped processing
    private AutoResetEvent _processingStopped = new AutoResetEvent(false);

    /// <summary>
    /// Called to start processing
    /// </summary>
    public void Start()
    {
        _stopProcessing.Reset();

        Thread thread = new Thread(ProcessRequests);
        thread.Start();
    }

    /// <summary>
    /// Called to request a graceful shutdown of the processing thread
    /// </summary>
    public void Stop()
    {
        _stopProcessing.Set();

        // Optionally wait for thread to terminate here
        _processingStopped.WaitOne();
    }

    /// <summary>
    /// This method does the actual work
    /// </summary>
    private void ProcessRequests()
    {
        WaitHandle[] waitHandles = new WaitHandle[] { _processingNeeded, _stopProcessing };

        Foo.RequestAdded += OnRequestAdded;

        while (true)
        {
            while (Foo.Requests.Count > 0)
            {
                string request;
                lock (Foo.Requests)
                {
                    request = Foo.Requests.Peek();
                }

                // Process request
                Debug.WriteLine(request);

                lock (Foo.Requests)
                {
                    Foo.Requests.Dequeue();
                }
            }

            if (WaitHandle.WaitAny(waitHandles) == 1)
            {
                // _stopProcessing was signalled, exit the loop
                break;
            }
        }

        Foo.RequestAdded -= ProcessRequests;

        _processingStopped.Set();
    }

    /// <summary>
    /// This method will be called when a new requests gets added to the queue
    /// </summary>
    private void OnRequestAdded()
    {
        _processingNeeded.Set();
    }
}


static class Foo
{
    public delegate void RequestAddedHandler();
    public static event RequestAddedHandler RequestAdded;

    static Foo()
    {
        Requests = new Queue<string>();
    }

    public static Queue<string> Requests
    {
        get;
        private set;
    }

    public static void AddRequest(string request)
    {
        lock (Requests)
        {
            Requests.Enqueue(request);
        }

        if (RequestAdded != null)
        {
            RequestAdded();
        }
    }
}

There are still a few problems with this, which I will leave to the reader:

• Checking for _stopProcessing should probably be done after every time a request is processed
• The Peek() / Dequeue() approach won't work if you have multiple threads doing processing
• Insufficient encapsulation: Foo.Requests is accessible, but Foo.AddRequest needs to be used to add any requests if you want them processed.
• In case of multiple processing threads: need to handle the queue being empty inside the loop, since there is no lock around the Count > 0 check.

Answer 3

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.

Answer 4

You are trying to remove objects from list as you are iterating through list. (OK, technically, you are not doing this, but that's the goal you are trying to achieve).

Here's how you do it properly: while iterating, construct another list of entries that you want to remove. Simply construct another (temp) list, put all entries you want to remove from original list into the temp list.

List entries_to_remove = new List(...);

foreach( entry in original_list ) {
   if( entry.someCondition() == true ) { 
      entries_to_remove.add( entry );
   }
}

// Then when done iterating do: 
original_list.removeAll( entries_to_remove );

Using "removeAll" method of List class.

Answer 5

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

Suppose you have two threads executing this code.

at System.Collections.Generic.List1[System.String]..ctor

• Thread1 starts processing the list.
• Thread2 calls the List constructor, which takes a count for the array to be created.
• Thread1 changes the number of items in the list.
• Thread2 has the wrong number of items.

Your locking scheme is wrong. It's even wrong in the for loop example.

You need to lock every time you access the shared resource - even to read or copy it. This doesn't mean you need to lock for the whole operation. It does mean that everyone sharing this shared resource needs to participate in the locking scheme.

Also consider defensive copying:

List<string> todos = null;
List<string> empty = new List<string>();
lock(Foo.Requests)
{
  todos = Foo.Requests;
  Foo.Requests = empty;
}

//now process local list todos

Even so, all those that share Foo.Requests must participate in the locking scheme.

Answer 6

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 7

The problem is the expression

new List<string>(Foo.Requests)

inside your foreach, because it's not under a lock. I assume that while .NET copies your requests collection into a new list, the list is modified by another thread

Answer 8

Three things:
- I wouldn't put them lock within the for(each) statement, but outside of it.
- I wouldn't lock the actual collection, but a local static object
- You can not modify a list/collection that you're enumerating

For more information check:
http://msdn.microsoft.com/en-us/library/c5kehkcz(VS.80).aspx

lock (lockObject) {
   foreach (string data in new List<string>(Foo.Requests))
        Foo.Requests.Remove(data);
}

Answer 9

To be completely honest, I would suggest refactoring that. You are removing items from the object while also iterating over that. Your loop could actually exit before you've processed all items.