How to deal with Concurrent Modification Exception

[Qunfeng Wang]

Exception in thread "Thread-7" java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:449)
at java.util.AbstractList$Itr.next(AbstractList.java:420)
at Main.CGuiUpdater.run(CGuiUpdater.java:139)
at java.lang.Thread.run(Thread.java:595)

It's a static List. Several threads poll this list to get some information but no modification. Whenever runing to CGuiUpdater.run, the runtime exception throws. Is it because I use Iterator to iterator List or something else? And how to resolve it, Thanks.

[Jim Yingst]

It's a combination of things - it's because you're using the Iterator at the same time another thread is modifying the list, and there's apparently no synchronization (or perhaps, insufficient or incorrect synchronization) to prevent these events from occurring near-simultaneously.

One solution is to synchronize all access to the List, using the list itself as the monitor:

synchronized (list) {
    for (Iterator it = list.iterator(); it.hashNext(); ) {
        Foo f = (Foo) it.next();
        // do what you need with f
    }
}

Whatever other threads are modifying the list, they need to be synchronized too, also on the list. E.g.:

syncronized (list) {
    list.add(new Foo());
}

or

syncronized (list) {
    list.remove(oldFoo);
}

Now this may be undesirable if the list is long and performance is critical, because if it takes a long time to iterate through the list, other threads are also blocked while waiting for this to complete. There are various alternatives. One is to use a CopyOnWriteArrayList. This is good if writes are fairly infrequent. You may also be able to revise your code in a way that it is not necessary to iterate an entire list. You may be able to use other data structures instead, such as ConcurrentHashMap or ConcurrentLinkedQueue. It's hard to say what would be most suitable here without more details on what you're doing.

[Qunfeng Wang]

Thanks for your reply.

The list is modified(add elements)only at the begining. The following code only get elements from it. So I'm thinking if I can make sure the modification operation occurs before getting operation, the exception won't happen?

[Jim Yingst]

Yes, that should be correct. If you're seeing this exception, then probably one thread is not done initializing the list before another thread checks it. Even if common sense is telling you it really should be done. Threads can do some strange things.

One approach is something like this:

class MyClass {
    private final List myList = makeList();
 
    private static list makeList() {
        List list = new ArrayList();
        // do what you need to initialize this
        return Collections.unmodifiableList(list);
    }
 
    public List getList() { 
       return myList; 
    }
}

Note that when you initialize the list, no other thread can possibly know about it yet. "list" is just a local variable, while "myList" does not yet know anything about the new list. Only after it's initialized does myList get the reference to the list. The fact that myList is declared final here is actually important for subtle reasons - otherwise the compiler or JIT may perform some weird optimizations that you only notice when multiple threads are involved. And then only rarely - but when they happen, they're a real pain to track down.

If making the variable final is not an option for you, there are other strategies. Maybe make it volatile - but then you have to worry about whether it's null, before you do anything with it. As it is now (with final), that's not possible.

For more extended discussion of this and many related threading issues, I heartily recommend Java Concurrency in Practice, an excellent use of time and money for any multithreaded programmer (at least if they're already pretty comfortable with the threading topics covered in the Java Tutorial).
[ August 29, 2007: Message edited by: Jim Yingst ]

[Billy Tsai]

Hello,

I have a method which retrieves a java.util.List of objects for display on the UI and this method is always access concurrently by hundred or even thousands of threads at the same time(high concurrency),
however if certain condition is true I need to remove some objects from that List before it is returned by that method, since this method has a very high usage I must take performance into account therefore
I can't use synchronized block in the method, I just want some confirmations I can't use Iterator.remove in a multi-thread situation but there should be no performance difference in directly looping through the
list using
(for int i=0; i<List.size(); i++) {List.get(i)}
and with
for (final Iterator i = temp.iterator(); i.hasNext();) ,
I have attached the following code snippets

    
public List getOrderedChildOfferProducts() {
//getDisplayableChildOfferProductInstances method retrieves a List of displayable products from the database through an ORM framework
        final List temp = new ArrayList(this.getDisplayableChildOfferProductInstances()); 
        final HttpSession session = request.getSession();
        final Plan currentPlan = (Plan)session.getObject("plan");
        if (currentPlan != null && !temp.isEmpty()) {
            final List products = currentPlan.getAllProducts();
            for (final Iterator i = temp.iterator(); i.hasNext();) {
              final Product product = (Product)i.next();
              if (!products.contains(product)) {
              	i.remove();
              }
            }
        }
        Sort.sort(temp, "displayOrder", true);
        return Collections.unmodifiableList(temp);
}

I want to confirm here in a multithreaded situation the i.remove() would not work in the above method?

my currently solution is as follows and would like to know if are there any advises to better improve this method for better efficiency since it is a critical method

 
public List getOrderedChildOfferProducts() {
//getDisplayableChildOfferProductInstances method retrieves a List of displayable products from the database through an ORM framework
        final List temp = new ArrayList(this.getDisplayableChildOfferProductInstances()); 
        final HttpSession session = request.getSession();
        final Plan currentPlan = (Plan)session.getObject("plan");
        if (currentPlan != null && !temp.isEmpty()) {
            final List removableProducts = new ArrayList();
            final List products = currentPlan.getAllProducts();
            for (final Iterator i = temp.iterator(); i.hasNext();) {
              final Product product = (Product)i.next();
              if (!products.contains(product)) {
              	removableProducts.add(op);
              }
            }
            temp.removeAll(removableProducts);
        }
        Sort.sort(temp, "displayOrder", true);
        return Collections.unmodifiableList(temp);
}

>

[Ori An]

There is another simple way to avoid this exception. This way does not involve any locks .

Instead of a simple iterator you can use a copy of the list and iterate the copy. See example here:

Dynamic Collection Iterator

[Dennis Hopfer]

I know this is an old topic but a simple, elegant solution is as follows:

for (Object instance : (ArrayList<Object>)list.clone())
{	
    list.remove(instance);
}

Since Java does shallow copying on clone() calls this seems to work fine and since they (original & clone) have different iterators there's no violation when the original is modified.

Happy coding!!!

[Jeff Verdegan]

qunfeng wang wrote:
It's a static List.

There is no such thing.

[Praveen Kumar M K]

Be wary of this - ConcurrentModificationException bug.

Not sure if its fixed in Java 7, however.

[Jeff Verdegan]

Praveen Kumar M K wrote:Be wary of this - ConcurrentModificationException bug.

Not sure if its fixed in Java 7, however.

True, but from the CCME docs:

Note that fail-fast behavior cannot be guaranteed [snip]. Fail-fast operations throw ConcurrentModificationException on a best-effort basis. Therefore, it would be wrong to write a program that depended on this exception for its correctness

So, yeah, on the 2nd to last iteration, Java may not do you the favor of telling you that you have a bug.

[Praveen Kumar M K]

Yes, and this behavior is with list.remove() only, that too if one were using foreach loop instead of iterator methods.

[Jeff Verdegan]

Praveen Kumar M K wrote:that too if one were using foreach loop instead of iterator methods.

A foreach loop does use an Iterator.

Notice how all 3 methods produce the same bytecode.

import java.util.*;

public abstract class Z {
  static List<String> x;

  static void foo() {
    for (String s : x) {
      s.length();
    }
  }

  static void bar() {
    for (Iterator<String> iter = x.iterator(); iter.hasNext();) {
      String s = iter.next();
      s.length();
    }
  }

  static void baz() {
    Iterator<String> iter = x.iterator();
    while (iter.hasNext()) {
      String s = iter.next();
      s.length();
    }
  }
}


:; javap -c -classpath . Z
Compiled from "Z.java"
public abstract class Z extends java.lang.Object{
static java.util.List x;

public Z();
  Code:
   0:   aload_0
   1:   invokespecial   #1; //Method java/lang/Object."<init>":()V
   4:   return

static void foo();
  Code:
   0:   getstatic       #2; //Field x:Ljava/util/List;
   3:   invokeinterface #3,  1; //InterfaceMethod java/util/List.iterator:()Ljava/util/Iterator;
   8:   astore_0
   9:   aload_0
   10:  invokeinterface #4,  1; //InterfaceMethod java/util/Iterator.hasNext:()Z
   15:  ifeq    36
   18:  aload_0
   19:  invokeinterface #5,  1; //InterfaceMethod java/util/Iterator.next:()Ljava/lang/Object;
   24:  checkcast       #6; //class java/lang/String
   27:  astore_1
   28:  aload_1
   29:  invokevirtual   #7; //Method java/lang/String.length:()I
   32:  pop
   33:  goto    9
   36:  return

static void bar();
  Code:
   0:   getstatic       #2; //Field x:Ljava/util/List;
   3:   invokeinterface #8,  1; //InterfaceMethod java/util/List.iterator:()Ljava/util/Iterator;
   8:   astore_0
   9:   aload_0
   10:  invokeinterface #4,  1; //InterfaceMethod java/util/Iterator.hasNext:()Z
   15:  ifeq    36
   18:  aload_0
   19:  invokeinterface #5,  1; //InterfaceMethod java/util/Iterator.next:()Ljava/lang/Object;
   24:  checkcast       #6; //class java/lang/String
   27:  astore_1
   28:  aload_1
   29:  invokevirtual   #7; //Method java/lang/String.length:()I
   32:  pop
   33:  goto    9
   36:  return

static void baz();
  Code:
   0:   getstatic       #2; //Field x:Ljava/util/List;
   3:   invokeinterface #8,  1; //InterfaceMethod java/util/List.iterator:()Ljava/util/Iterator;
   8:   astore_0
   9:   aload_0
   10:  invokeinterface #4,  1; //InterfaceMethod java/util/Iterator.hasNext:()Z
   15:  ifeq    36
   18:  aload_0
   19:  invokeinterface #5,  1; //InterfaceMethod java/util/Iterator.next:()Ljava/lang/Object;
   24:  checkcast       #6; //class java/lang/String
   27:  astore_1
   28:  aload_1
   29:  invokevirtual   #7; //Method java/lang/String.length:()I
   32:  pop
   33:  goto    9
   36:  return

}

[Praveen Kumar M K]

I suspected that there would be a reply on similar lines

What I meant with regards to CCME, was this

    int indexCount = 0;
    for (Iterator<String> iter = list.iterator(); iter.hasNext();iter.next()) {   
         if(indexCount == (LENGTH - 2)) {
              list.remove(indexCount);
         }
         indexCount++;
    } 

Above code produced the CCM exception. I suppose we can attribute it to the non-deterministic behavior as the doc suggests.

[Bhanuprakash Sreenivas]

I had a similar problem with one of the method variables. Spent hours debugging . Finally had to use Collections.unmodifiableList to
retrieve a unmodifiable list.
Feel it has more to do with the JRE being used.

Thanks

[Jeff Verdegan]

Dennis Hopper wrote:I know this is an old topic but a simple, elegant solution is as follows:

for (Object instance : (ArrayList<Object>)list.clone())
{	
    list.remove(instance);
}

Since Java does shallow copying on clone() calls this seems to work fine and since they (original & clone) have different iterators there's no violation when the original is modified.

Not sure what the point of that is supposed to be.

First, it runs in O(n^2).

Second, it accomplishes nothing more than list.clear().

[Jeff Verdegan]

I had a similar problem with one of the method variables. Spent hours debugging . Finally had to use Collections.unmodifiableList to
retrieve a unmodifiable list.
Feel it has more to do with the JRE being used.

Sorry, I'm confused. What problem exactly did you have? And how did using unmodifiableList() solve it? And why do you think it's a JRE-specific issue?

[Dennis Hopfer]

for (Object instance : (ArrayList<Object>)list.clone())  
{     
    list.remove(instance);  
}  

Since evidently it requires explanation I will explain. This isn't necessarily slower than a normal for-loop; just make the clone call and save it to a variable.

ArrayList<Object> clone = (ArrayList<Object>)list.clone();
for (Object instance : clone)  
{     
    list.remove(instance);  
}  

And this code's applicability is not limited to clearing an array. Using this technique one can use all the functions of the List class and without the liability of having to use the ugly Iterator interface. And since you aren't modifying the original you don't need to worry about throwing ConcurrentModificationException instances. This should be pretty obviously the correct solution for this problem.

[Jeff Verdegan]

Dennis Hopper wrote:

for (Object instance : (ArrayList<Object>)list.clone())  
{     
    list.remove(instance);  
}  

Since evidently it requires explanation I will explain. This isn't necessarily slower than a normal for-loop;

It may not necessarily be slower than a poorly written for loop, but it's definitely slower than a normal one.. As I said previously: It's O(n^2) and it accomplishes only list.clear(), which is O(1).

just make the clone call and save it to a variable.

ArrayList<Object> clone = (ArrayList<Object>)list.clone();
for (Object instance : clone)  
{     
    list.remove(instance);  
}  

That's identical to the original. Introducing the variable changes nothing.

And this code's applicability is not limited to clearing an array. Using this technique one can use all the functions of the List class and without the liability of having to use the ugly Iterator interface.

It's nonstandard, confusing code that serves no purpose. Iterator isn't particularly ugly. There are cases where it makes sense to operate on a copy of the original, but there's nothing about your example that makes any sense.

And since you aren't modifying the original

In the example you gave you certianly are. You're clearing it the very, very long way around.

you don't need to worry about throwing ConcurrentModificationException instances. This should be pretty obviously the correct solution for this problem.

There are already better and easy-to-use approaches to avoiding CME. Whatever the original problem was, I guarantee this is not even remotely a correct solution.