1

I have a class which stores data and gets called from many threads. It fails with a ConcurrentModificationException although every access to my Set is synchronized.

How can this happen? The synchronized should make sure that my Set is not changed while it is iterated...

Here are all functions from my class that access the Set...

Can anyone tell me what is going wrong here?

private final Object mListenerLock = new Object();
private final Set<IRetainerBaseListener> mListeners = new HashSet<IRetainerBaseListener>();

protected final void register(IRetainerBaseListener listener)
{
    synchronized (mListenerLock)
    {
        mListeners.add(listener);
    }
}

protected final boolean unregister(IRetainerBaseListener listener)
{
    synchronized (mListenerLock)
    {
        return mListeners.remove(listener);
    }
}

private final void onObjectAdded(RKey key, Object data)
{
    synchronized (mListenerLock)
    {
        Iterator<IRetainerBaseListener> it = mListeners.iterator();
        while (it.hasNext())
        {
            IRetainerBaseListener listener = it.next();

            /* EDIT */
            /* I'm not changing the Set in here, never!!! */

            // I can't insert the if's, but I just check the interface class
            // and call one of the following methods:

            ((IRetainerListener) listener).onRetainerDataAdded(key, data);
            // or
            ((IRetainerSingleKeyListener) listener).onRetainerDataAdded(data);

        }
    }
}
prom85
  • 16,896
  • 17
  • 122
  • 242
  • 1
    You're adding or removing from the set while iterating it. You should do the removal through the `Iterator`. – Sotirios Delimanolis Oct 02 '13 at 13:35
  • that should not happen because of the lock or am I wrong? – prom85 Oct 02 '13 at 13:36
  • `ConcurrentModificationException` doesn't occur because of `concurrent` thread access. – Sotirios Delimanolis Oct 02 '13 at 13:37
  • To clarify where the problem lies, can you show the code inside your `while` loop. That's where a possible `ConcurrentModificationException` could happen without involving mutliple threads. – Jonathan Drapeau Oct 02 '13 at 13:47
  • 1
    You only need one thread to get a CME. If you modify it in one thread without using the Iterator, it can fail. – Peter Lawrey Oct 02 '13 at 13:51
  • I added the two interface functions which could be called, depending on the interface class in the list... stackoverflow does not allow me to insert the if's... don't know why... – prom85 Oct 02 '13 at 13:53
  • 1
    [Read this, you'll understand why 1 thread can cause the error](http://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html) – Jonathan Drapeau Oct 02 '13 at 13:59
  • I understand why this can happen in one thread if I edit the list while iterating. And I read the link. Do you mean, the link should explain me why this can happen, if I DON'T change the list while iterating? – prom85 Oct 02 '13 at 14:20
  • I think that all your thread use the same "object lock" instance (mListenerLock) and hence manage to get the lock thanks to reentrant synchronization as stated in @JonathanDrapeau 's link. Can you try to use synchronized methods instead of synchronized blocks? – Arnaud Denoyelle Oct 02 '13 at 14:23
  • ok, now I understand what you mean... I will try your recommendation... and to the reentrant synchronization: I thought, this can only happen if the SAME thread tries to acquire a lock again. Otherwise, the lock would not make much sense, would it? – prom85 Oct 02 '13 at 14:30
  • making the 3 functions synchronized does not solve the problem either... – prom85 Oct 02 '13 at 14:36
  • 1
    `this can only happen if the SAME thread`. True, I actually misunderstood this part. Can you try to add traces "begin/end of iteration" and traces on synchronized methods? I would like to be sure that there is no call to `(un)register()` while iterating. – Arnaud Denoyelle Oct 02 '13 at 14:39
  • you are right, actually this happens... But I think with that in my mind I can find the error. All the tipps helped me at least, so thanks... I think my error is, that I'm not registering my listeners in the thread, but before creating one... so I'm quite sure now, that the reentrant synchronization is my problem... – prom85 Oct 02 '13 at 15:12
  • If you find the problem, please tell us. Someone seems to have the same problem here : http://stackoverflow.com/questions/19164796/java-concurrentmodificationexception-even-with-synchronized – Arnaud Denoyelle Oct 03 '13 at 17:14
  • I can't wirte an answer here... but my problem was, that I was calling register BEFORE creating a thread. The thread afterwards called the function with the iterator. And then the main thread registered the second listener while the first thread was iterating. So my solution was, that I do EVERYTHING in the threads and never do something to the list from the main thread. – prom85 Oct 03 '13 at 20:03
  • PS: it does not really make sense, only, and only if I explain it with that reentrant synchronization is allowed for a child thread... i don't know if this is possible though... – prom85 Oct 03 '13 at 20:07

2 Answers2

4

It's not a problem of thread safety.

You are removing items while iterating on your collection. This is only possible using an iterator.

 /*
  * *it* does not appreciate that you removed elements 
  * in another way than it.remove();
  * The iterator must do the add/remove operations itself
  * to guarantee that it will not break the iteration.
  */
 while (it.hasNext()) {
   IRetainerBaseListener listener = it.next();
   ...
 }
Arnaud Denoyelle
  • 29,980
  • 16
  • 92
  • 148
  • how can I remove objects from the collection, if the remve/add functions have to wait for the lock? Iteration must be finished before I add/remove an object or isn't that true? – prom85 Oct 02 '13 at 13:38
  • 1
    You have 2 choices : either you remove elements using the iterator (but you can only remove the current element). Or you keep a list of items to remove and you remove them all at the end of the iteration. – Arnaud Denoyelle Oct 02 '13 at 13:39
  • 1. I would recommend using java.util.concurrent classes if you know you are running MT, they'll do the synchronization stuff for you. 2. you can iterate through your collection, items to remove put to a new list, and remove them all after the iteration is finished. Arnaud is right, your problem is not the MT environment – Jan Hruby Oct 02 '13 at 13:41
  • actually, I don't understand how add/remove can be called while iterating. I'm definitely not modifying my Set while iterating it. So just another thread could edit it. And this should not happen, because of the lock. My understanding is, that if I don't change the Set in my iteration, the Set can't be changed while iteration because of the lock. Isn't that true? – prom85 Oct 02 '13 at 13:45
  • Unsure why people vote this up as the code in the `while` loop isn't present and, from the OP latest edit, he doesn't change the `Set` in it. – Jonathan Drapeau Oct 02 '13 at 13:50
  • @prom85 I think that it would work with synchronized methods. But I don't know how behave synchronized blocks inside a method. If you want to be sure to avoid the CME, you can still do a defensive copy of your collection before itearting on it. But I don't like this solution because you will do a lot of useless copies – Arnaud Denoyelle Oct 02 '13 at 13:57
0

Synchronized is ensuring that no other thread is executing while also trying to hold a 'lock' during that operations.

Example:

Thread A: synchronized(mListenerLock) { doSomething; }

Thread B:

synchronized(mListenerLock) { doSomething; }

This way either A or B is doing something. One have to wait for the other to release the binary 'lock' of mListenerLock.

In your case you use the very same thread to perform your actions. So you get the concurrent modification exception because you alter the state of the list (removing objects) while also iterating it.

ConcurrentModificationException does not refer to a concurrency problem in terms of threads. It is just stating that while doing one thing with a list (or related objects) your program does some other things that prevents things from working as intended. It is a (defensive) safety mechanism to prevent common bugs to happen unnoticed.

Martin Kersten
  • 5,127
  • 8
  • 46
  • 77