Arraylist throws ConcurrentModificationException when it has a high number of values

Question

So here is my issue, I have an ArrayList, it contains all of the entities that should be rendered to the screen.

It does so like this with a foreach loop.

for (Entity e : entities) {
    g.fillRect(x, y, w, h);
}

This works perfectly fine with no errors when it is populated with a lower numbers of values such as 50. That is, the size of the list is 50. However when it is something like 1,000 it throws a ConcurrentModificationException and crashes the app.

I know that the exception means the list has been modified whilst iterating through it, but in that loop is never actually does anything to the list. The list is accessed elsewhere to update things, but shouldn't the foreach loop finish before something else happens that modifies the list?

The list is modified in an update method which updates entities.

The zombies are enemies in a sense and survivors are AI. When a zombie collides with an AI it removed the survivor and replaces it with a survivor. This is the only place the lists are modified.

This all works perfectly when it is dealing with a small number of entities, however with a larger number it crashes.

public void update(double delta) {
    for (Zombie z : zombies) {
        z.update(delta);
    }
    for (Survivor s : survivors) {
        s.update(delta);
    }

    List<Survivor> toRemove = new ArrayList<Survivor>();
    List<Zombie> toAdd = new ArrayList<Zombie>();

    for (Survivor s : survivors) {
        for (Zombie z : zombies) {
            if (z.collides(s)) {
                toAdd.add(new Zombie(s.position, this, zms));
                toRemove.add(s);
            }
        }
    }

    for (Survivor s : toRemove) {
        survivors.remove(s);
    }

    for (Zombie z : toAdd) {
        zombies.add(z);
    }
}

Is this a multi-threaded application? My suspicion is that the higher numbers are causing errors because it gives more time for another thread to come in and change something — Cruncher, Nov 25 '13 at 21:52
The body of the loop is only a Graphics.fillRect call to draw a rectangle. It's not multi-threaded so I'm not sure what could be causing. As the list is not modified there. It's modified elsewhere, but that stuff can't be running during the foreach loop, correct? — Darren, Nov 25 '13 at 21:55
@Darren can you give more information about how it's modified elsewhere? And you should post the code in the loop. It would also be helpful if you could post an SSCCE http://sscce.org/. If where it's being edited is in any event, then yes it will be run in another thread. Java has its own thread for events — Cruncher, Nov 25 '13 at 21:56
`public void update(double delta)` Are you sure this method is not running in another thread? It sounds like it is.. — Cruncher, Nov 26 '13 at 13:33

Cruncher · Accepted Answer · 2013-11-28T13:41:54.143

public void update(double delta)

This method sounds like one that's being called by an engine of some sort, which is almost certainly in another thread.

for (Entity e : entities) {
    g.fillRect(x, y, w, h);
}

This is running in your swing thread, which is separate.

When you have a few number of entities it is likely that this operation may complete atomically. When you have a much larger number of entities, you have a much higher chance of swapping into another thread to do work in the middle of drawing the entities.

The fix(I'm assuming zombies and survivors are entities):

synchronized(entities)
{
    for (Survivor s : toRemove) {
        survivors.remove(s);
    }

    for (Zombie z : toAdd) {
        zombies.add(z);
    }
}

And in your paint:

synchronized(entities)
{
    for (Entity e : entities) {
        g.fillRect(x, y, w, h);
    }
}

This will ensure that only 1 thread can be in one of the synchronized blocks at a time, forcing them to happen separate from eachother

EDIT: This has a possibility of painting a frame after a collision has occurred. If your frame rate is high enough this will be completely unnoticeable. If you do start to notice it, then you may need to do a little bit more work so that once an update starts, a paint will not start until completely done.

Thanks for the help. I actually figured it out on my own but I used about the same thing you posted. I didn't think about the graphics calls running through the swing thread rather than the games. Thanks for the info though, helped to clarify some things. — Darren, Nov 28 '13 at 05:25
@Darren glad I could help :). A lot of people have multi-threaded applications when they don't even know it! This can especially throw people off when they explicitly call the `repaint()` method, not realising that this just sets a flag telling the swing thread that it should paint. — Cruncher, Nov 28 '13 at 13:45

score 2 · Answer 2 · answered Nov 26 '13 at 13:55

2

To avoid synchronization, you should think about using the list read-only.

That is, in update(), don't remove from the list, but copy survivors to a new list, then add zombies to that new list. Finally, you can replace the reference to the old list with one to the new list. This only requires synchronization on the object that holds this reference. But because the replacement of a reference is really cheap, synchronization wouldn't cause another trhread to wait too long.

// pseudo code
newList = update(entities);    // takes some time
synchronized (this) {
    entities = newList;        // is quasi-immediate
}

Don't forget to make the getter for entities synchronized, too, and access entities only through the getter.

answered Nov 26 '13 at 13:55

Ingo

36,037
5
53
100

Actually, personally I would use a LinkedList for this task. Random access is never used here. – Cruncher Nov 26 '13 at 13:59
@Cruncher Me too. Yet we don't know whether indexed access is used somewhere else. But it seems that ArrayList is favored among Java programmers, for whatever reason. (Note that my post speaks of lists only in a general sense) – Ingo Nov 26 '13 at 14:03
I assume that with this, you're also going to synchronize around the paint loop? If not, then you don't need the synchronized block here at all. `entities=newList;` is also atomic I would think. – Cruncher Nov 26 '13 at 14:05
As I said, I expect that the paint loop gets the entity reference through a synchronized getter. This reference, once gotten, remains valid (though it doesn't necessarily reflect the latest state), and so the paint loop can run without synchronization. – Ingo Nov 26 '13 at 14:08
My point is that I don't even think the synchronized getter is necessary. As long as your get is atomic and doesn't do any extra work. I think this solution works without synchronization at all – Cruncher Nov 26 '13 at 14:10
I think you can shorten this to `entities = update(entities);` And then in your paint at the top `List localEntities = entities;` and then use that. Would this not work? Completely synchronization free. – Cruncher Nov 26 '13 at 14:23

Arraylist throws ConcurrentModificationException when it has a high number of values

2 Answers2