C#: How can I solve "Collection was modified" in a BackgroundWorker progress report callback?

Question

I've used BackgroundWorkers quite a bit, but I've never experienced this problem before. My program analyses the output from a logic analyser producing packets, of which there are thousands. To prevent too much delay updating the ListView in my form (I was previously reporting each one as it was found, and the form was completely unresponsive) I'm collecting the packets inside the BackgroundWorker in a generic list (List<Packet>) and then reporting that either when n amount are found (currently 250), or when an exception occurs, or when it completes.

The problem occurs in my callback when I'm iterating over the List<Packet> I get an InvalidOperationException, with the "Collection was modified" error. I'm not touching the collection inside the foreach (I'm adding to another collection, but I see no reason this could modify the collection I'm iterating over - plus commenting it out doesn't fix the problem.) I've even tried locking the e.UserState, storing the e.UserState into a local scope List<Packet> and locking that, nothing seems to work.

Here's the code for my callback method:

void worker_ProgressChanged(object sender, ProgressChangedEventArgs e)
{
    progressBar.Value = e.ProgressPercentage;
    packetsListView.SuspendLayout();
    lock ((List<Packet>)e.UserState)
    {
        foreach (Packet packet in (List<Packet>)e.UserState)
        {
            packets.Add(packet);
            ListViewItem item = new ListViewItem(string.Format("{0}ns", Math.Round(packet.StartSampleNumber * 41.666667)));
            item.Tag = packet;
            item.SubItems.Add(new ListViewItem.ListViewSubItem(item, packet.Description));
            packetsListView.Items.Add(item);
        }
    }
    packetsListView.ResumeLayout();

    statusLabel.Text = string.Format("Analyzing...found {0} {1}", packetsListView.Items.Count, packetsListView.Items.Count == 1 ? "packet" : "packets");
}

Answer 1

You cannot modify a collection on which you are iterating with a foreach. You are calling the packets.Add method inside the foreach loop and I suspect that this packets variable points to the same collection you are iterating over.

If this is not the case you may try locking on a private static field that you declare inside your form:

private static object _syncRoot = new object();

and then:

lock (_syncRoot)
{
    ...
}

Answer 2

A simple explanation for your problem is that you use lock in the ProgressChanged event handler but not in the DoWork event handler. Which lets the worker thread still modify the collection while the UI thread is iterating it.

It is simple to solve, just create a new List<> right after you call ReportProgress in the worker. The UI thread is now the only one that has a reference to the list, you don't need to use lock anymore.

Answer 3

I found that if performance is not a big issue, then using some concurrent-safe collection works pretty well. More info here

Answer 4

Create packet in this method, and then it can't possibly alias the one iterated through.