ConcurrentModicationException due to unsynchronized synchronized methods

Question

I have a bunch of Runnable objects that are launched from a base class. These objects are iterating through and deleting items in a shared ArrayList at random times. I have synchronized both methods but am getting a ConcurrentModicationException. I believe its because they are synchronized but are not synchronized on each other. Is this the case? If so, which class do I acquire the lock on?

Environment Class:

class Environment{

     ArrayList<Critter> critter_array = new ArrayList<Critter>();
     ArrayList<Food> food_array = new ArrayList<Food>();

     Environment(){
         Executor ex = Executors.newCachedThreadPool(); 
         Critter A = new Critter();
         Critter B = new Critter();
         critter_array.add(A);
         critter_array.add(B);
         ex.execute(A); 
         ex.execute(B);
     }

     public synchronized void destroyFood(Food item){
         Iterator<Food> iter = food_array.iterator();
         while(iter.hasNext()){
             Food temp = iter.next();
             food_array.remove(temp);
             break;
         }      
     }

}

Critter class:

class Critter implements Runnable{
     Environment envi;

     Critter(Environment E){
          envi = E;
     }

     @Override
     public void run() {
         //do other stuff
         CritterUtilities.iteratorMethod(this, envi);
     }
}

CritterUtilities class:

class CritterUtilities{

    public static synchronized iteratorMethod(Critter self, Environment envi){
         Iterator<OtherObject> iter = envi.getFood().listIterator();
         while(iter.hasNext()){
             Food temp = iter.next();   /////////Problem is here
             //other stuff that's not related to the prob
        }
    }

}

Stack Trace:

Exception in thread "pool-1-thread-2" java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(Unknown Source)
at java.util.AbstractList$Itr.next(Unknown Source)
at CritterUtil.getViewableFood(CritterUtil.java:28)
at CritterUtil.getNearestFood(CritterUtil.java:41)
at Critter.hunt(Critter.java:163)
at Critter.run(Critter.java:139)
at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
at java.lang.Thread.run(Unknown Source)

This concurrentModificationException you get normally if you remove stuff while looping over it. Are you sure you don't call xxx.remove() somewhere inside a loop? — d0x, Aug 05 '13 at 15:57

DaoWen · Accepted Answer · 2013-08-05T16:59:07.800

4

The error doesn't necessarily stem multithreading here, but rather the fact that you're removing items from the list while using an iterator:

 public synchronized void destroyFood(Food item){
     Iterator<Food> iter = food_array.iterator();
     while(iter.hasNext()){
         Food temp = iter.next();
         food_array.remove(temp); // <-- this is the problem
         break;
     }      
 }

Instead, use the Iterator.remove() method:

 public synchronized void destroyFood(Food item){
     Iterator<Food> iter = food_array.iterator();
     while(iter.hasNext()){
         iter.remove(); // <-- this is how you remove elements while iterating
         break;
     }      
 }

On another note, your synchronization is wrong. Each of your synchronized methods is syncing on a different object. To get them to all synchronize on the same object it would probably be easiest to just have them all synchronize on the list itself.

edited Aug 05 '13 at 16:59

answered Aug 05 '13 at 15:58

DaoWen

32,589
6
74
101

Question, as I've never used this class before: does that remove the element from the original (in this case, `food_array`) List? – Deactivator2 Aug 05 '13 at 16:03
1

@Deactivator2 - updated my answer. The documentation says it removes it from the _list_, not just from the _iterator_. – DaoWen Aug 05 '13 at 16:07
1

You don't need `ListIterator`. The only thing it adds over regular `Iterator` is that you can iterate in both directions. `remove` is the same for both. – zapl Aug 05 '13 at 16:21
@zapl - You're right. I even double-checked that and still missed it somehow... I updated my answer. – DaoWen Aug 05 '13 at 16:59

kiheru · Answer 2 · 2013-08-05T16:28:46.910

In addition to the faulty removal spotted by DaoWen, the synchronization is wrong with too. iteratorMethod() and destroyFood() do not use the same object for sychronization. Currently they use their respective objects for synchronization. It should be something like this instead:

 public void destroyFood(Food item) {
     synchronized (food_array) {
         // ... proceed with the removal
     }
 }

and

public static iteratorMethod(Critter self, Environment envi){
    List<OtherObject> list = envi.getFood();
    synchronized (list) {
        // the rest of iteratorMethod should go here
    }
}

(I'm assuming getFood() returns the food array, so that then the objects would be the same).

The same fix should be applied to any other methods modifying, or iterating the the food list.

ConcurrentModicationException due to unsynchronized synchronized methods

2 Answers2