HashMap insert during iteration

Question

I maintain a variable number of event listeners in a Map:

private final Map<String, EventListener> eventListeners = new ConcurrentHashMap<>();

The class where the map is used has a method to add an event listener:

public void addEventListener(final String name, final EventListener listener) {
    eventListeners.put(name, listener);
}

Every time an event occurs I iterate over all listeners and fire them:

eventListeners.forEach((name, listener) -> {
    listener.handle(event);
});

The environment is single-threaded and the part where it becomes interesting is that event listener implementations may insert another event listener when they are fired.

With an "ordinary" HashMap, this would obviously lead to a ConcurrentModificationException, this is why I am using a ConcurrentHashMap.

What I see happening is that an event listener that is inserted by another event listener may be fired by the same event since it is inserted during iteration, which I think is the expected behaviour of ConcurrentHashMap.

However, I don't want this to happen, so I would like to defer any insertion of an event listener until iteration over all listeners has completed.

So I introduced a Thread that waits for the iteration to complete before it inserts a listener using a CountDownLatch:

public void addEventListener(final String name, final EventListener listener) {
    new Thread(() -> {
        try {
            latch.await();
        } catch (InterruptedException e) {}
        eventListeners.put(name, listener);
    }).start();
}

and the iteration over all listeners:

latch = new CountDownLatch(1);
eventListeners.forEach((name, listener) -> {
    listener.handle(event);
});
latch.countDown();

It works as expected but I wonder if this is a good solution at all and if there are more elegant ways to achieve what I need.

score 1 · Answer 1 · answered Aug 09 '14 at 14:58

I haven't analyzed your issue fully, but the normal way to do this is to use a CopyOnWriteArraYList.

public class CopyOnWrite {
   private CopyOnWriteArrayList<ActionListener> list = 
           new CopyOnWriteArrayList<>();

   public void fireListeners() {
      for( ActionListener el : list ) 
         el.actionPerformed( new ActionEvent( this, 0, "Hi" ) );
   }

   public void addListener( ActionListener al ) {
      list.add( al );
   }
}

A CopyOnWriteArrayList is thread safe and also won't throw ConcurrentModificationException.

Link

CopyOnWriteArrayList is a List implementation backed up by a copy-on-write array. This implementation is similar in nature to CopyOnWriteArraySet. No synchronization is necessary, even during iteration, and iterators are guaranteed never to throw ConcurrentModificationException. This implementation is well suited to maintaining event-handler lists, in which change is infrequent, and traversal is frequent and potentially time-consuming.

I think using CopyOnWriteArrayList is a nice suggestion, but in this case I need a map. — Torsten Römer, Aug 09 '14 at 15:24

Marco Acierno · Accepted Answer · 2014-08-09T15:10:58.970

If your solution let you to implement a Thread something could be wrong.

You can avoid to iterate over the original map and iterate over a copy of it.

So something like:

Map<String, EventListener> eventListenerCopy = new HashMap<>(eventListeners);
eventListenerCopy.forEach((name, listener) -> {
  listener.handle(event);
});

so if your handle event will add new events it will not be see by the copy which will iterate over all the original events.

Or, if you force a specific method to add new listeners you can create a temp list where all the new listeners will be saved and when the iteration finish check this new list and if present put the new listeners in the original lists (this could be better if you don't want to create a copy of the map everytime).

My question and your answer is a nice example for that probably always a simple solution is way better that a complicated one. — Torsten Römer, Aug 09 '14 at 15:35

score 1 · Answer 3 · edited May 23 '17 at 12:29

It works as expected but I wonder if this is a good solution at all and if there are more elegant ways to achieve what I need.

It is elegant (I guess) but it is not a good solution. If I understand what you are doing here, you are spawning a thread each time you want to add a listener. This is incredibly expensive.

In a typical JVM, a thread has a large off-heap memory segment that holds the thread stack. Each time you start a thread, a syscall is made to allocate the memory, and other syscalls are made to create and start the native thread.

At any rate, according this Q&A - Java thread creation overhead - it takes in the region of 0.1 milliseconds to create and start a thread.

I can think of two ways to do this:

Instead of adding the new listeners to eventListeners during the iteration, add them to a temporary list, and then append that to the main eventListeners map after you've finished firing the events.
Put the listeners straight into the eventListeners map, but implement something to prevent them from firing early. For example, a per-listener flag or an integer-valued activation field that you compare against an event serial number.

(Others have suggested using a CopyOnWriteList, and that's a good suggestion unless the eventListener list / map is liable to be big and/or listeners are liable to be added frequently.)

I went for Marco Acierno's suggestion which came a bit earlier as yours which is similar, but you have a really good point about the performance implication. — Torsten Römer, Aug 09 '14 at 15:29

HashMap insert during iteration

3 Answers3