Android: How to correctly synchronize iteration over Set when another thread can remove elements from the Set?

Question

Why does a call to removeListener() in the following code throw a ConcurrentModificationException when another thread is using the iterator in fireEvent()?

public class MyClass {

    private Set<Object> synchronizedListeners;

    public MyClass() {
        synchronizedListeners = Collections.synchronizedSet(
                new LinkedHashSet<Object>());
    }

    public void addListener(Object listener) {
        synchronizedListeners.add(listener);
    }

    public synchronized void removeListener(Object listener) {
        synchronizedListeners.remove(listener);
    }

    public void fireEvent() {
        synchronized (synchronizedListeners) {
            for (Object listener : synchronizedListeners) {
                // do something with listener
            }
        }
    }
}

From my understanding, since I'm using synchronized (synchronizedListeners) in fireEvent(), this should block any other thread that calls removeListener(), till the iteration in fireEvent() is complete at which point it should be safe to remove an element from this Set. But this doesn't seem to be the case. What am I doing wrong?

Possibly related: Java synchronized block vs. Collections.synchronizedMap

Edit: It was pointed out that I was unnecessarily synchronizing the removeListener() method. So I tried this version:

public void removeListener(Object listener) {
    synchronizedListeners.remove(listener);
}

But still got the same error.

Edit 2: As assylias pointed out, the problem isn't visible in the above code. I was calling removeListener() from inside the for loop in the synchronized (synchronizedListeners) block which was causing the error. The fix I ended up using in this case is to remove the listener from another thread:

public void removeListener(final Object listener) {
    new Thread() {
        @Override
        public void run() {
            synchronizedListeners.remove(listener);
        }
    }.start();
}

`synchronizedListeners` is protected - do you have any subclasses to MyClass? do they override any of the methods? — assylias, Aug 07 '12 at 09:35
No. I made an edit and changed it to private to make that clearer. — Alinium, Aug 07 '12 at 09:58
Your code as it is can't throw a `ConcurrentModificationException`. So the problem is not shown in your question: you might have simplified your code too much before posting and removed the source of the problem. I suggest you try to post a [Short, Self Contained, Correct Example (SSCCE)](http://www.sscce.org/) that demonstrates your problem. — assylias, Aug 07 '12 at 10:05
You don't call `set.remove()` or `removeListener()` within your `fireEvent` loop, do you? — assylias, Aug 07 '12 at 10:06
Yes, I was calling `removeListener()` from inside `fireEvent`! It was hidden a few layers inside some calls, so I missed it. — Alinium, Aug 07 '12 at 11:56

score 4 · Answer 1 · answered Aug 07 '12 at 09:35

4

You are synchronizing on two different objects.

The removeListener method is synchronized on the MyClass instance, whereas the loop inside the fireEvent is synchronized on synchronizedListeners set.

What you need to do is to synchronize every method that uses the synchronizedListeners on the set itself.

answered Aug 07 '12 at 09:35

npe

15,395
1
56
55

1

`synchronizedListeners` is a synchronized collections: the remove method is already synchronized. The `removeListener` method does not need to be synchronized but adding synchronization on the set should not make a difference... – assylias Aug 07 '12 at 09:39
It will. If both blocks (the remove(), and the loop) are synchronized on the same lock, they will never get executed concurrently for the same object. That's how critical sections work. – npe Aug 07 '12 at 09:41
remove **IS ALREADY** synchronized internally: `synchronizedListeners = Collections.synchronizedSet(new LinkedHashSet – assylias Aug 07 '12 at 09:42
But the iterator created from the SynchronizedSet is not. `SynchronizedSet` inherits from `SynchronizedCollection` and the implementation of `iterator` method [does not return synchronized iterators](http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/Collections.java#Collections.SynchronizedCollection). You need to synchronize them yourself. – npe Aug 07 '12 at 09:46
yes and he does that: the iteration is made in fireEvent and is synchronized with the right monitor (the set itself). – assylias Aug 07 '12 at 09:47
The proper link: [http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/Collections.java#Collections.SynchronizedCollection](http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/Collections.java#Collections.SynchronizedCollection). It even says: `return c.iterator(); // Must be manually synched by user!` – npe Aug 07 '12 at 09:48
I know - I'm saying that he does synchronize the iteration properly: `synchronized (synchronizedListeners) { for (Object listener : synchronizedListeners) {...} }` – assylias Aug 07 '12 at 09:51
OK. You're right, and I'm blind. But in this case, why does he get `ConcurrentModificationException`? – npe Aug 07 '12 at 09:52
The problem is that `SynchronizedCollection.remove()` uses a different (internal) lock object than his synchronized `for` loop. So it's not synchronized at all. – Martin Majer Aug 07 '12 at 09:54
@npe That's the question. There is nothing in there that could do that. Unless there is a subclass that does something weird for example... – assylias Aug 07 '12 at 09:54
@MartinMajer remove uses the set itself as a lock (`mutex` in the code). – assylias Aug 07 '12 at 09:55
1

@MartinMajer: You're making the same mistake as I did. The internal lock (`mutex` variable) is set to `this` in the constructor. See the links I provided. – npe Aug 07 '12 at 09:55

score 3 · Accepted Answer · answered Aug 07 '12 at 09:57

I can't reproduce what you describe - the code at the bottom gives the output shown below - which shows that remove is called in the middle of the iteration, but does not complete until after the iteration because you use a synchronized collection. That's the behaviour one would expect and no ConcurrentModificationException is thrown. Note that I have removed the synchronized keyword from the removeListener method as it is useless here.

fire 100000
remove 100000
done fire 100000
done remove 99999

Conclusion: the problem is somewhere else. For example, if you have a subclass that overrides the fireEvent method, or if you construct the synchronized set not exactly as in the code you posted.

public static void main(String[] args) {
    final MyClass mc = new MyClass();
    final Object o = new Object();
    mc.addListener(o);
    for (int i = 0; i < 99999; i++) {
        Object o1 = new Object();
        mc.addListener(o1);
    }
    Runnable remove = new Runnable() {

        @Override
        public void run() {
            mc.removeListener(o);
        }
    };

    new Thread(remove).start();
    mc.fireEvent();
}

public static class MyClass {

    protected Set<Object> synchronizedListeners = Collections.synchronizedSet(new LinkedHashSet<Object>());

    public void addListener(Object listener) {
        synchronizedListeners.add(listener);
    }

    public void removeListener(Object listener) {
        System.out.println("remove " + synchronizedListeners.size());
        synchronizedListeners.remove(listener);
        System.out.println("done remove " + synchronizedListeners.size());
    }

    public void fireEvent() {
        System.out.println("fire " + synchronizedListeners.size());
        synchronized (synchronizedListeners) {
            for (Object listener : synchronizedListeners) {
                // do something with listener
            }
        }
        System.out.println("done fire "  + synchronizedListeners.size());
    }
}

Android: How to correctly synchronize iteration over Set when another thread can remove elements from the Set?

2 Answers2