LinkedList modified, Thread crashing the program

Question

My problem is a synchronization problem with a thread and the user simultaneously accessing and modifying a LinkedList.

I’m making a program in C# that will display some messages in a panel. I’m getting an error called “The collection was modified after the enumerator was instantiated.”, that is because I’m adding or removing messages while a thread is accessing the LinkedList.

I have read some solutions but I am still unable to make them work. I’m using an Enumerator for the thread work in my LinkedList. I tried to make some locks in my code so the thread would not iterate the list while I remove or add an element. I also tried to lock the thread for the operations on my list. But all my attempts failed.

Here is some code of my project. This one is for adding a message:

public void addMsg(MsgForDisplay msg) {
    Label lbl = new Label();
    lbl.Text = (msg.getMsgText() + " -");
    lbl.ForeColor = color;
    lbl.Font = textFont;
    lbl.BackColor = backg;
    lbl.Visible = true;
    lbl.AutoSize = true;
    lbl.Location = new Point(width(), 0);
    //lock(labels) { tried to lock here but failed
        labels.AddLast(lbl);
        lastLb = lbl;
        this.Controls.Add(lbl);
        this.Refresh();
    //}
}

Removing a message:

public void removeMsg(string msg) {
    string remove = msg + " -";
    Label lbRemove = null;
    //lock(labels) { also tried to lock here
        var it = labels.GetEnumerator();
        while(it.MoveNext()) {
            Label label = it.Current;
            if (label.Text.Equals(remove)) {
                lbRemove = label;
            }
        }
        labels.Remove(lbRemove);
        this.Controls.Remove(lbRemove);
        this.Refresh();
    //}
}

And there is the problem, in my thread:

public void run() {
    while (true) {
        // lock (labels) { also tried to lock here
            var it = labels.GetEnumerator();
            while (it.MoveNext()) { // the crash occurs here
                Label lb = it.Current;
                if (lb.Location.X + lb.Width < 0) {
                    this.Invoke(new MethodInvoker(() => { this.Controls.Remove(lb); }));
                    if (labels.Count > 1)
                        this.Invoke(new MethodInvoker(() => { lb.Location = new Point(lastLb.Right, 0); }));
                    else
                        this.Invoke(new MethodInvoker(() => { lb.Location = new Point(2000, 0); }));
                    lastLb = lb;
                    this.Invoke(new MethodInvoker(() => { this.Controls.Add(lb); }));
                    this.Invoke(new MethodInvoker(() => { this.Refresh(); }));
                }
                if (leftLb != null)
                    if (leftLb.Location.X + leftLb.Width - lb.Location.X < -20)
                        this.Invoke(new MethodInvoker(() => { lb.Location = new Point(leftLb.Right, 0); }));
                    else
                        this.Invoke(new MethodInvoker(() => { lb.Location = new Point(lb.Location.X - 3, lb.Location.Y); }));
                    leftLb = lb;
            }
            System.Threading.Thread.Sleep(30);
        // }
    }
}

As you can see I’m using an GetEnumerator of my labels, what in Java should be the Iterator. With this I shouldn’t be able to iterate the list without problem when the user add or remove messages?

Is there a way to synchronize the accesses to the list?

EDIT: I have tried the ConcurrentBag and ConcurrentDictionary but without any improvement to the project as you can see in the comments…

Please before you post an answer read the comments bellow to make sure that you know what is going on.

EDIT: Tried to add a mutex to my code for addMsg and removeMsg but still crashing. If I use the mutex in the thread it will be slowed down.

Answer 1

I created a Timer in step of the thread and that solved the crashing problem. Here is the code if you want to see it.

private System.Windows.Forms.Timer myTimer = new System.Windows.Forms.Timer();

private void startThread() {
    myTimer.Tick += new EventHandler(timerEvent);
    myTimer.Interval = 30;
    myTimer.Start();
}

private void timerEvent(object sender, EventArgs e) {
    var it = labels.GetEnumerator();

    while (it.MoveNext()) {
        Label lb = it.Current;
        // Label lb = labels.ElementAt(b);
        if (lb.Location.X + lb.Width < 0) {
            this.Controls.Remove(lb);
            if (labels.Count > 1)
                lb.Location = new Point(lastLb.Right, 0);
            else
                lb.Location = new Point(2000, 0);
            lastLb = lb;
            this.Controls.Add(lb);

            this.Refresh();
        }

        if (leftLb != null)
            if (leftLb.Location.X + leftLb.Width - lb.Location.X < -20)
                lb.Location = new Point(leftLb.Right, 0);
            else
                lb.Location = new Point(lb.Location.X - 3, lb.Location.Y);
        leftLb = lb;
    }
}

Answer 2

The source of your problem is that while you are iterating over the list of labels You call either Remove or Add functions which modifies this list whis is not allowed while iterating over it. Instead of this

var it = labels.GetEnumerator();
while (it.MoveNext()) // the crash occurs here

I suggest something like that:

for(int i = 0; i < labels.Count; i++)
{
     labels.remove(labels[i]); //this is valid of course the count of the list will change
     //Here you can add or remove elements from the labels
}

Or you can try first to collect the removable items into a temporal list and later remove it from the original.

Answer 3

As others have already stated, the problem is you are modifying the collection while enumerating over it.

Now, the easiest workaround is obviously not to enumerate over the same collection that is being modified. And how do you do that? Simple, you just clone the collection, and iterate over it:

lock (labels)
{
    var clone = new LinkedList<Label>(labels);
    it = labels.GetEnumerator();
}

Now you can enumerate over it safely, without worrying about inconsistencies.

A few notes:

• I am using a lock, because the cloning also must enumerate over your collection, and while it does it in a very short time, it is still required for synchronization. Off course, you need to uncomment the locks you've already added to addMsg and removeMsg.
• The reason that locking your whole loop didn't work, is that when you call Invoke, you are essentially returning control to the thread that owns the object (the main GUI thread in this case). The problem is, that this thread is already stuck on handling whatever event caused addMsg or removeMsg to be called, leading to a deadlock.
• You should also note that cloning a collection every 30 ms, isn't exactly efficient, and shouldn't be used in a production code, but given that this probably just an exercise, it should suffice. In real life, you should probably use a separate collection for the changes you are about to do (adding or removing labels), change this collection in addMsg and removeMsg, and then merge the changes to labels inside your thread, but outside of the iteration over the labels.
• Not directly related to your question, but still: you should use a foreach loop instead of directly creating an enumerator object in C#.

Answer 4

As stated before, changing any collection while enumerating it, results in an exception in .Net. You can avoid this by using for or while loops.

However I don't see the point in using a Linked List in this scenario. It should be way simpler and more performant to use a ConcurrentDictionary and just add or remove the requested item. There is also a ObservableConcurrentDictionary available, although not part of the Framework. It is very stable, in my experience. http://www.codeproject.com/Articles/208361/Concurrent-Observable-Collection-Dictionary-and-So