Collection was modified in foreach loop C#

Question

I know there are MANY similiar questions, but I can't seem to get to the bottom of this.

In my program I execute a verification method which should compare two ascii HEX files with eachother (one is local, the other is read from a USB device). Some code:

private void buttonVerify_Click(object sender, EventArgs e)
{
    onlyVerifying = true;

    Thread t = new Thread(verifyProgram);
}
private void verifyProgram()
{
    verifying = true;
    externalFlashFile.Clear();

    // After this method is finished, the returned data will end up in 
    // this.externalFlashFile since this listen to the usb's returned data
    hexFile.readExternalFlashForVerify(usbDongle, autoEvent);

    externalFlashFile.RemoveAt(0);
    //externalFlashFile.RemoveAt(externalFlashFile.Count - 1);
    hexFile.verifyProgram(externalFlashFile);
}

public void verifyProgram(List<string> externalProgram)
{
    byte[] originalFile = null; // Will be modified later with given size
    byte[] externalFile = new byte[4096];
    int k = 0, errors = 0;

    // Remove last line which contains USB command data
    externalProgram.RemoveAt(externalProgram.Count - 1);

    foreach (String currentLine in externalProgram)
    {
        for (int i = 0; i < 64; i += 2)
        {
            string currentDataByte = currentLine.Substring(i, 2); 
            externalFile[k] = Convert.ToByte(currentDataByte, 16);
            k++;
        }
        progress += steps;
    }
//... compare externalFile and originalFile

When executing the readExternalFlashForVerify the USB is responding with requested data. This data is parsed and calls an eventhandler:

public void usbDongle_OnDataParsed(object sender, EventArgs e)
{
  if (verifying)
  {
      usbDongle.receivedBytesString.Trim();
      externalFlashFile.Add(usbDongle.receivedBytesString.Substring(2, 32 * 2));
      // Allow hexFile continue its thread processing
      autoEvent.Set();
  }
}

The first run is always completes correctly. The following executions, at the third or fourth iteration of the foreach, I get an extra element in externalProgram. This is not a global variable (argument in function call) and the function is not called anywhere else. This ofcourse throws an exception.

I tried adding .ToList() to externalProgram in the foreach but that didn't do any difference. How can my externalProgram be modified during this execution?

EDIT: I never found the cause of this, but replacing the foreach with a hard-coded for-loop solved the issue at hand. Not an optimal solution, but don't have much time on this.

// The list should never be larger than 128 items
for (int j = 0; j < 0x7f ; j++)
{
     string currentLine = externalProgram[j];
     // ...

Is this all the code? What calls `verifyProgram`? Possibly something is already iterating `externalProgram` before it is given to `verifyProgram` and the `RemoveAt` call makes it pop. The `foreach` in the provided code looks fine on the surface, I cannot see where that error could come from in that. — Adam Houldsworth, Nov 26 '13 at 11:41
There must be something wrong with the code you posted. This code wouldn't throw the error, as the `externalProgram` list isn't touched by the code you've shown. — Thorsten Dittmar, Nov 26 '13 at 11:42
@AdamHouldsworth No, not all code. I tried only posting the essentials. What do you mean by `RemoveAt` makes it pop? I get an extra element, no elements removed. — chwi, Nov 26 '13 at 11:44
@JeffRSon Yes, but no other calls are made to the function. This function indeed works in its own thread — chwi, Nov 26 '13 at 11:44
@ThorstenDittmar Good to get confirmation of that. Since it's my company's code I can't post much more than I already have :\ — chwi, Nov 26 '13 at 11:45
@Wilhelmsen If you call any mutating methods on a list that is currently being enumerated it will throw the exception you are getting, which is what I mean by "makes it pop". `RemoveAt` could cause your problem if something is actively iterating `externalProgram` at the time. — Adam Houldsworth, Nov 26 '13 at 11:45
Well, `externalProgram` is not a copy of the list. It's a reference to the list used when calling. So if you modify that, your `externalProgram` is modified as well. What else are you doing with the list? — JeffRSon, Nov 26 '13 at 11:45
Let’s start at the basics: *Where* does the error occur? Please post the full stack trace. — poke, Nov 26 '13 at 11:50
@JeffRSon I haven't thought about that. The only thing I do is read line from line from USB, store this in the list, remove the first element and pass it to the function. By now knowing it is a reference this could be a cause why it always executes correctly the first time. I will look more into that. Thanks! — chwi, Nov 26 '13 at 11:50
You should post more code, especially how you start the thread and how the starter continues with the list. — JeffRSon, Nov 26 '13 at 11:51
Well, it may be that `usbDongle_OnDataParsed` is called again while your verification runs. Try logging or setting a breakpoint. — JeffRSon, Nov 26 '13 at 12:54
@Wilhelmsen My best guess is that the event was firing on a different thread, so would add to the list while you were iterating it. A `for` loop might "work", but you will get non-deterministic results if the length of the list underneath changes as you index out of it. — Adam Houldsworth, Nov 26 '13 at 14:59

score 0 · Answer 1 · edited May 23 '17 at 10:31

0

Usually when you receive an exception with a message like that it is caused by multiple accesses from different threads to a list. What I suggest you is to use a lock when you add and remove items from that list, so you're sure the indexes to that collection are not changing. You have to think what would happen if you try to remove the last element (of index 3, for example) of a collection when someone else removes a previous item (changing the lenght of the collection to 3...).

This example: Properly locking a List<T> in MultiThreaded Scenarios? describes better what I mean.

edited May 23 '17 at 10:31

Community

1
1

answered Nov 26 '13 at 11:51

Mattia Vitturi

235
4
17

http://msdn.microsoft.com/en-us/library/system.collections.concurrent.aspx Should do the trick. – Bauss Nov 26 '13 at 13:43

score 0 · Answer 2 · answered Nov 26 '13 at 11:51

0

Probably this line is a problem:

externalProgram.RemoveAt(externalProgram.Count - 1);

If verifyProgram is called multiple times, it will remove more and more lines from externalProgram list passed by reference

answered Nov 26 '13 at 11:51

Andriy Tylychko

15,967
6
64
112

As written in the comments the probably is an *additional* element. – JeffRSon Nov 26 '13 at 11:55
Yes, I get an extra element. But I see now that this may cause other issues – chwi Nov 26 '13 at 11:56

Collection was modified in foreach loop C#

2 Answers2