0

I have a Java 1.8 class that holds two collections:

Map<Key,Object>
Set<Object>

My class five methods:

addObjectToMap()
removeObjectFromMap()
addObjectToSet()
removeObjectFromSet()

loopOverEverything(){
    for(Object o : mySet){
        for(Object o2 : myMap.getKeySet()){
            doSomething(o,o2);
        }
    } 
}

The point of the class is to implement the observer pattern but be very flexible in both observers and observees. The problem I face is that the last method can easily throw a ConcurrentModificationException when a thread calls the add/remove methods while the looping is going on. I was thinking about synchronizing all methods on "this":

set.add(object);

would become

synchronized(this){
    set.add(object);
}

and I would add a similar synchronize statement to the other 4 methods, including the loop method.

Will this work? I know synchronizing methods can cause bottlenecks. Even though there is currently no performance issue, I would like to rationalize this design and hear about possible alternatives that have less of a performance hit.

Improve this question
1
  • 1
    @AndyTurner That probably won't solve the problem - Collections.synchronizedSet doesn't hold a lock during iteration, so you can still get ConcurrentModificationException fairly easily. Commented Sep 13, 2016 at 22:37

3 Answers 3

Reset to default
2

No, it's not safe unless the loop is also synchronized. If you want to avoid the overhead of locking for the duration of each loop, consider using concurrent collections, such as ConcurrentHashMap and Collections.newSetFromMap(new ConcurrentHashMap<>()).

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Are you saying a concurrent hashmap also locks on iterating over the keyset of that map?
@user1884155 It doesn't lock, but it's guaranteed to be thread-safe. See the documentation for details.
How should I interpret this message in the documentation: "They do not throw ConcurrentModificationException. However, iterators are designed to be used by only one thread at a time." What do they mean by "are designed to"? If I remove the synchronize statements in the looping method but use a concurrenthashmap, what will happen if two threads start iterating simultaneously?
@user1884155 You're misunderstanding. It's just saying that concurrent threads should not share a single iterator, not that they can't iterate concurrently over the same map.
1

Because locks are re-entrant, synchronizing access to the collection won't stop the same thread iterating over a collection, and then modifying the collection itself. Despite the name, ConcurrentModificationException is not always raised due to concurrent modification by another thread. Registering or un-registering other observers in a notification callback is a prime culprit for such concurrent modification.

A common strategy for firing events is to take a "snapshot" of the listeners to be notified before delivering any events. Because the order in which observers are notified is generally unspecified, it's valid for all observers that were registered when an event is generated to receive it, even if another observer de-registers it as a result of that notification.

To make a snapshot, you can copy the observers to a temporary collection or array:

Collection<?> observers = new ArrayList<>(mySet);

or 

Object[] observers = mySet.toArray(new Object[mySet.size()]);

Then iterate over that copy, leaving the original available for update:

for (Object o : observers) {
  ...
    doSomething(o, ...);
  ...
}

Some concurrent collections like ConcurrentSkipListSet won't raise an exception, but they only guarantee "weakly consistent" iteration. This could result in some (newly added) observers unexpectedly receiving the current notification. CopyOnWriteArraySet uses a snapshotting technique internally. If you rarely modify the set, it will be more efficient, because it only copies the array when necessary.

Improve this answer

4 Comments

This post has some good examples of weakly consistent iteration.
I understand why one thread can in theory access both the add/remove methods and the loop methods simultaneously, but I don't understand how one thread, that runs sequentially over the code, could over perform both. Like, when it's adding an objec, it cannot be looping, because it's a single thread. Can a single thread be "split up" by the cpu/cores into doing different tasks at the same time?
@user1884155 No. It's not that complicated. It's just a loop that modifies the collection. Like in your original example, suppose your doSomething() method adds an element to mySet. That can raise a ConcurrentModificationException. The thread is only performing one action at a time: check to see if there's another element; if so, advance to it; then process it; then repeat. The problem is if "process it" results in modifying the collection, the "check to see if there's another element" that happens on the next repeat will detect the collection has changed and raise an exception.
I ended up removing the synchronizing blocks and using the idea of taking snapshots of the collections before I start the loop.
0

One possible answer that will perform better than just using synchronized blocks is to change your collections to thread-safe ones. Instead of HashMap (or whatever map you're using), use ConcurrentHashMap. Replace your Set with either a CopyOnWriteArraySet or a ConcurrentSkipListSet. You'll need to read the documentation on those to make sure their threading behavior is what you actually want, but both should be an improvement over a thread-unsafe class with no synchronization.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.