CODEWITHSUNDEEP
iteratorarraylistconcurrentmodificationconcurrent-collections
Snailer
Snailer

Reputation: 3839

Another ConcurrentModificationException question

I've searched StackOverflow and there are many ConcurrentModificationException questions. After reading them, I'm still confused. I'm getting a lot of these exceptions. I'm using a "Registry" setup to keep track of Objects:

public class Registry {
    public static ArrayList<Messages> messages = new ArrayList<Messages>();
    public static ArrayList<Effect> effects = new ArrayList<Effect>();
    public static ArrayList<Projectile> proj = new ArrayList<Projectile>();

    /** Clears all arrays */
    public static void recycle(){
        messages.clear();
        effects.clear();
        proj.clear();
    }
}

I'm adding and removing objects to these lists by accessing the ArrayLists like this: Registry.effects.add(obj) and Registry.effects.remove(obj)

I managed to get around some errors by using a retry loop:

//somewhere in my game..
boolean retry = true;
while (retry){
    try {
        removeEffectsWithSource("CHARGE");
        retry = false;
    }
catch (ConcurrentModificationException c){}
}

private void removeEffectsWithSource(String src) throws ConcurrentModificationException {
    ListIterator<Effect> it = Registry.effects.listIterator();
    while ( it.hasNext() ){
        Effect f = it.next();
        if ( f.Source.equals(src) ) {
            f.unapplyEffects();
            Registry.effects.remove(f);
        }
    }
}

But in other cases this is not practical. I keep getting ConcurrentModificationExceptions in my drawProjectiles() method, even though it doesn't modify anything. I suppose the culprit is if I touched the screen, which creates a new Projectile object and adds it to Registry.proj while the draw method is still iterating.

I can't very well do a retry loop with the draw method, or it will re-draw some of the objects. So now I'm forced to find a new solution.. Is there a more stable way of accomplishing what I'm doing?

Oh and part 2 of my question: Many people suggest using ListIterators (as I have been using), but I don't understand.. if I call ListIterator.remove() does it remove that object from the ArrayList it's iterating through, or just remove it from the Iterator itself?

Upvotes: 2

Views: 501

Answers (2)

andersoj
andersoj

Reputation: 22884

Top line, three recommendations:

  • Don't do the "wrap an exception in a loop" thing. Exceptions are for exceptional conditions, not control flow. (Effective Java #57 or Exceptions and Control Flow or Example of "using exceptions for control flow")
  • If you're going to use a Registry object, expose thread-safe behavioral, not accessor methods on that object and contain the concurrency reasoning within that single class. Your life will get better. No exposing collections in public fields. (ew, and why are those fields static?)
  • To solve the actual concurrency issues, do one of the following:
    1. Use synchronized collections (potential performance hit)
    2. Use concurrent collections (sometimes complicated logic, but probably efficient)
    3. Use snapshots (probably with synchronized or a ReadWriteLock under the covers)

Part 1 of your question

You should use a concurrent data structure for the multi-threaded scenario, or use a synchronizer and make a defensive copy. Probably directly exposing the collections as public fields is wrong: your registry should expose thread-safe behavioral accessors to those collections. For instance, maybe you want a Registry.safeRemoveEffectBySource(String src) method. Keep the threading specifics internal to the registry, which seems to be the "owner" of this aggregate information in your design.

Since you probably don't really need List semantics, I suggest replacing these with ConcurrentHashMaps wrapped into Set using Collections.newSetFromMap().

Your draw() method could either a) use a Registry.getEffectsSnapshot() method that returns a snapshot of the set; or b) use an Iterable<Effect> Registry.getEffects() method that returns a safe iterable version (maybe just backed by the ConcurrentHashMap, which won't throw CME under any circumstances). I think (b) is preferable here, as long as the draw loop doesn't need to modify the collection. This provides a very weak synchronization guarantee between the mutator thread(s) and the draw() thread, but assuming the draw() thread runs often enough, missing an update or something probably isn't a big deal.

Part 2 of your question

As another answer notes, in the single-thread case, you should just make sure you use the Iterator.remove() to remove the item, but again, you should wrap this logic inside the Registry class if at all possible. In some cases, you'll need to lock a collection, iterate over it collecting some aggregate information, and make structural modifications after the iteration completes. You ask if the remove() method just removes it from the Iterator or from the backing collection... see the API contract for Iterator.remove() which tells you it removes the object from the underlying collection. Also see this SO question.

Upvotes: 2

Dan Dyer
Dan Dyer

Reputation: 54475

You cannot directly remove an item from a collection while you are still iterating over it, otherwise you will get a ConcurrentModificationException.

The solution is, as you hint, to call the remove method on the Iterator instead. This will remove it from the underlying collection as well, but it will do it in such a way that the Iterator knows what's going on and so doesn't throw an exception when it finds the collection has been modified.

Upvotes: 1

Related Questions