ConcurrentModificationException when multiple thread access the same Collection

Question

I have 2 inner thread classes of class Main. Sometimes, It causes ConcurrentModificationException when one add new element while another is removed. I think I don't know how to synchronize them.

Class Main{
HashSet<MyObject> set;   
Thread A{
       run(running){
          ...
          set.add(obj);
          ...
       }
    }

Thread B{
     run(){
      while (running) {
                for (Iterator<MyObject> i = set.iterator(); i.hasNext();) {
                    MyObject obj= i.next();
                    if (!obj.isSmt()) {
                        i.remove();
                       ...
                    }
                }
            }
     }
}

}

Answer 1

The simplest solution is to isolate the reading code from the writing code. You would do that by surrounding the modifications with synchronized(set) blocks. For the first call, we must synchronize around the add call:

run(running){
...
    synchronized(set) {
        set.add(obj);
    }
...
}

For the second call, we need to synchronize around the entire iteration to avoid concurrent modification. i.remove() is correct in a single threaded case, but as you've discovered, it doesn't work across multiple threads.

synchronized(set) {
    for (Iterator<MyObject> i = set.iterator(); i.hasNext();) {
        MyObject obj= i.next();
        if (!obj.isSmt()) {
            i.remove();
           ...
        }
    }
}

synchronized(set) is a lock on the object set. Only one thread will be able to enter either of the synchronized blocks at a given time, preventing items from being added to the set while a thread is iterating over it.

Answer 2

The ConcurrentModificationException is caused by set.add(obj) in ThreadA while the iteration is in progress in ThreadB (and not by the set.remove() during the loop).

The threads need to be synchronized in order to avoid this.

Threads are synchronized using intrinsic locks over some object. You declare this using the 'synchronized' keyword:

// entire method synchronized on 'this'
synchronized SomeValue foo();

// block synchronized on obj:
synchronized( obj ) {
    // stuff.
}

Details vary a lot depending on what you need synchronized. In the case of collections, it is generally safe to isolate specific operations like add() or remove(), but if you need to iterate over the elements in the collection, you need to synchronize the entire block that will carry the iteration if you use regular collection implementations:

synchronized( set ) {
    for (Iterator<MyObject> i = set.iterator(); i.hasNext();) {
        ...
    }
}

However, it is generally possible to implement more efficient synchronizations depending on the nature of the collection, and it is very easy to make errors when synchronizing by hand. For most cases, it is generally preferable to just use one of the collection implementations found in java.util.concurrent, which implement iterators that are thread-safe and would not throw a ConcurrentModificationException from operations from a different thread.

For some reason, there's no ConcurrentHashSet implementation of Set, but it is possible to obtain an instance of one by using newSetFromMap:

HashSet<MyObject> set = Collections.newSetFromMap( new ConcurrentHashMap<MyObject,Object>() );