Best way to prevent concurrent modification exception

Question

Here is some pseudo code as follows.

public class MyObject
{   
    private List<Object> someStuff;
    private Timer timer;

    public MyObject()
    {
        someStuff = new ArrayList<Object>();

        timer = new Timer(new TimerTask(){

            public void run()
            {
                for(Object o : someStuff)
                {
                    //do some more stuff involving add and removes possibly
                }
            }
        }, 0, 60*1000);
    }

    public List<Object> getSomeStuff()
    {
        return this.someStuff;
    }
}

So essentially the problem is that other objects not listed in the code above call getSomeStuff() to get the list for read-only purposes. I am getting concurrentmodificationexception in the timer thread when this occurs. I tried making the getSomeStuff method synchronized, and even tried synchronized blocks in the timer thread, but still kept getting the error. What is the easiest method of stopping concurrent access of the list?

Answer 1

You can use either java.util.concurrent.CopyOnWriteArrayList or make a copy (or get an array with Collection.toArray method) before iterating the list in the thread.

Besides that, removing in a for-each construction breaks iterator, so it's not a valid way to process the list in this case.

But you can do the following:

for (Iterator<SomeClass> i = list.iterator(); i.hasNext();) {
    SomeClass next = i.next();
    if (need_to_remove){
       i.remove(i);                
    }
}

or

for (int i = list.size() - 1; i >= 0; i--){            
    if (need_to_remove) {
        list.remove(i);                
    }
}

Also note, that if your code accesses the list from different threads and the list is modified, you need to synchronize it. For example:

    private final ReadWriteLock lock = new ReentrantReadWriteLock();


    final Lock w = lock.writeLock();
    w.lock();
    try {
        // modifications of the list
    } finally {
        w.unlock();
    }

      .................................

    final Lock r = lock.readLock();
    r.lock();
    try {
        // read-only operations on the list
        // e.g. copy it to an array
    } finally {
        r.unlock();
    }
    // and iterate outside the lock 

But note, that operations withing locks should be as short as possible.

Answer 2

You should make a copy of the list in getSomeStuff(). Publishing a reference to a private field like this makes it effectively public, so it isn't something you want to do anyway.

Also, consider returning a copy as an ImmutableList or at least as an unmodifiable list.