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?

Eugene Retunsky · Accepted Answer · 2012-04-19T14:41:29.133

14

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.

edited Apr 19 '12 at 14:41

answered Apr 18 '12 at 22:20

Eugene Retunsky

13,009
4
52
55

But, the work done in the thread (add and removes) have to actually persist in the list object. I just don't want the other threads accessing the list for read purposes (foreach loop) to cause the JVM to freak out. – thatidiotguy Apr 19 '12 at 14:17
I guess I just feel that doing something with Java's synchronize keyword would have been possible, but you are saying this is not the case? And what packages are these Lock and ReadWriteLock in? Also, I just need to lock the list itself while the run method in the timer is being executed, is this the approach you recommend? – thatidiotguy Apr 19 '12 at 14:44
Correct. The way you iterate/remove would break in a single thread environment. The lock class in java.util.concurrent package. You lock not the list, but access to it. I.e. operations with the list are performed inside the locks (though there is difference between read and write operation - so readers won't block each other). – Eugene Retunsky Apr 19 '12 at 14:53
I just do not see anything in the code that mentions the list at all. You are just building this lock object, but the list is never mentioned. Is it preventing ANY writing on any object? – thatidiotguy Apr 19 '12 at 15:21
There are comments in the blocks explaining what should be inside. – Eugene Retunsky Apr 19 '12 at 15:26

score 4 · Answer 2 · answered Apr 18 '12 at 22:23

4

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.

answered Apr 18 '12 at 22:23

Adam Zalcman

26,643
4
71
92

What I am doing right now is returning Collections.synchronizedList (which is not working). Objects that receive the list should be able to iterate through it for read purposes, but that is it. Would one of these solve the concurrentmodificationexception issue? Point taken on object security though. – thatidiotguy Apr 19 '12 at 14:20

Best way to prevent concurrent modification exception

2 Answers2