ConcurrentModificationException GL2/update thread, with sync and iterators

Question

I'm pretty sure I'm missing a minor thing, but I can't figure this out. I was following all the documentations, and searched for a solution, but I'm still getting this error.

The scene is the following: I have a display thread (openGL), and logics update thread. Both threads are iterating thru an arraylist, which contains the child nodes. These child nodes can have other child nodes added to them. If you are familiar with the cocos2d family and it's scenegraph, it's trying to be a copy of it.

Declaration on top of the CNode class:

private List<CNode> m_children;

Allocating and creating

this.m_children = Collections.synchronizedList(new ArrayList<CNode>());

Trying to draw

 public void visitdraw(GL2 gl){
    synchronized(this.m_children){
        this.draw(gl);
        Iterator<CNode> it = m_children.iterator();
        while (it.hasNext()){
        it.next().visitdraw(gl);
        }
    }
}

and to update

 public void visitupdate(){
    synchronized(this.m_children){
        this.update();
        Iterator<CNode> it = m_children.iterator();
        while (it.hasNext()){
            it.next().visitupdate();
        }
    }
}

and the problem occurs when I want to remove one (even if its not tested, I'm pretty sure it would raise the same exception if I wanted to add a new node runtime)

public void removeChild(CNode child){
    synchronized(this.m_children){
        Iterator<CNode> it = this.m_children.iterator();
        while(it.hasNext()){
            if (it.next().equals(child)){
                it.remove();
                break;
            }
        }
    }
}

I am fully aware that this system is not the best way to handle anything, but I'm literally forced to use this code. I just can't figure it out where the actual problem is.

Any help would be appreciated and welcomed, since for me it's kinda hard to understand why the previous developers went with this.

Answer 1

Problem

If Im not mistaken - the Problem here is, that you have two synchronized blocks

• one for iterating
• and a second for removing elements.

Both blocks are locked only for itself. It is not the List what is locked like you might know it from DB-Recources and DB-Transactions.

So you can call two different synchronized blocks at the same time. If you do that on the same Object, you end up with to concurrent calls on the same Object.

for Example:

private List list 

public void synchronized read() {
   ...iterate over list
}

public void synchronized remove() {
   ... remove some elements in list
}

And you have two Threads A and B

Then A and B cannot call read() at the same time and A and B cannot call remove at the same time. But they can call read() and remove() at the same time.

Solution

Try to use java.util.concurrent.locks.ReentrantReadWriteLock instead of the keyword synchronized. The Lock-Interface is new since Java 1.5 and is the modern way to do synchronization.

The following shows a class that own a buffer that will be read and changed by 3 Threads parallel:

class Container {

    private List<String> buffer = Collections.synchronizedList(new ArrayList<String>());
    private ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true);
    private Lock writeLock = lock.writeLock();
    private Lock readLock = lock.readLock();

    public void readBuffer() {
        readLock.lock();

        Iterator<String> it = buffer.iterator();
        while(it.hasNext()) {
            it.next();
        }

        readLock.unlock();
    }

    public void addOne() {
        writeLock.lock();

        buffer.add("next");

        writeLock.unlock();
    }

    public void removeOne() {
        writeLock.lock();

        if (buffer.size() > 0) {
            buffer.remove(0);
        }

        writeLock.unlock();
    }
}

For testing you can remove the readLock. That will lead to a ConcurrentModificationException. The following main() starts the test-Threads:

public class Concurrent {

    public static Container container = new Container();


    public static void main(String[] args) {
        new Thread(new Filler()).start();
        new Thread(new Killer()).start();
        new Thread(new Reader()).start();
    }

    static class Filler implements Runnable {
        @Override
        public void run() {
            while(true) {
                container.addOne();
            }
        }
    }

    static class Killer implements Runnable {
        @Override
        public void run() {
            while(true) {
                container.removeOne();
            }
        }
    }

    static class Reader implements Runnable {
        @Override
        public void run() {
            while(true) {
                container.readBuffer();
            }
        }
    }
}

The point is, that you use the same LockObject writeLock in two methods. Additionally this kind of Lock allows it to read the Data parallel. If it is necessary to allow only one Thread at a time to read the Data you could use ReentrantLock instead of ReentrantReadWriteLock