Should listeners be able to remove listeners?

Question

Many classes use code similar to the following to fire listeners.

private List<Listener> listeners = new ArrayList<Listener>();

public void fireListener() {
  for(Listener l : listeners) l.someMethod();
}

This works fine until the listener tries to add/remove a listener. This modification from inside the list causes a ConcurrentModificationException. Should we handle this case or should modifying listeners be invalid? What would be the best way to handle add/remove of listeners?

Update:
Here is one possible solution.

public void fireListener() {
  for(Listener l : listeners.toArray(new Listener[listeners.size()])) {
    if(!listeners.contains(l)) continue;
    l.someMethod();
  }
}

Answer 1

The right way to handle the addition/removal of listeners while browsing them would be to use an iterator that supports this.

For example:

// Assuming the following:
final List<Listener> listeners = ...

final Iterator<Listener> i = listeners.iterator();
while (i.hasNext()) {
   // Must be called before you can call i.remove()
   final Listener listener = i.next(); 

   // Fire an event to the listener
   i.notify();

   if (/* true iff listener wants to be deregistered */) {
       i.remove(); // that's where the magic happens :)
   }
}

NB: Please note that certain Iterators do not support the Iterator#remove method!

Answer 2

I would say it isn't a very good idea to allow listeners to add/remove other listeners (or themselves). It indicates a poor separation of concerns. After all, why should the listener know anything about the caller? How would you test such a tightly coupled system?

Something you can do instead is have your event handling method (in the listener) return a boolean flag that indicates that the listener doesn't want to receive more events. That makes it the responsibility of the event dispatcher to do the removal, and it covers most use cases where you need to modify the list of listeners from within a listener.

The major difference in this approach is that the listener simply says something about itself (i.e. "I don't want more events") rather than being tied to the implementation of the dispatcher. This decoupling improves testability and doesn't expose the internals of either class to the other.

public interface FooListener {
    /**
     * @return False if listener doesn't want to receive further events.
     */
    public boolean handleEvent(FooEvent event);
}

public class Dispatcher {

    private final List<FooListener> listeners;

    public void dispatch(FooEvent event) {
      Iterator<FooListener> it = listeners.iterator();
      while (it.hasNext()) {
        if (!it.next().handleEvent(event))
          it.remove();
      }
    }
}

Update: Adding and removing other listeners from within a listener is slightly more problematic (and should set off even louder alarm bells) but you can follow a similar pattern: the listener should return information on what other listeners need to be added/removed and the dispatcher should act on that information.

However in this scenario you get quite a few edge cases:

• What should happen with the current event?
• Should it be dispatched to the newly added listeners?
• Should it be dispatched to the ones about to be removed?
• What if you're trying to remove something that is earlier in the list and the event has already been sent to it?
• Also, what if listener X adds listener Y and then listener X is removed? Should Y go with it?

All these problems come from the listener pattern itself and the basic assumption of it that all the listeners in the list will be independent of each other. And the logic to handle these cases should definitely go in the dispatcher and not in the listener.

Update 2: In my example I used a naked boolean for brevity but in real code I'd define a two-valued enum type to make the contract more explicit.

Answer 3

There are three cases:

1. You don't want to allow the modification of the listeners collection during listeners execution:
   A ConcurrentModificationException would be appropriate in this case.

2. You want to allow modification of listeners, but the changes are not to be reflected in the current run:
   You have to make sure a modification of listeners does not have impact on the current run. A CopyOnWriteArrayList does the trick. Before using it read the API, there are some pitfalls.
   Another solution would be copying the list before iterating through it.

3. You want changes to listeners reflect within the current run:
   The most tricky case. Using for-each loops and iterators won't work here (as you have noticed ;-)). You have to keep track of changes yourself.
   A possbile solution would be to store listeners in an ArrayList and go through that list using a standard for loop:

   for (int i =0; i < listeners.size(); i++) { 
       Listener l = listeners.get(i);
       if (l == null) 
           continue;
       l.handleEvent();
   }  
   

   Removing listeners would set the element at its place in the array to null. New listeners are added to the end and therefore will run in the current execution.
   Notice that this solution is just an example and not threadsafe! Maybe some maintainance is necessary to remove null elements sometimes to keep the list from growing too big.

It is up to you to decide what is needed.

My personal favourite would be the second one. It allows modifications while execution but does not change the current run's behaviour which can cause unexpected results.

Answer 4

You can't add/remove to a list/map that's being used. In C# there's a nice little class called ConcurrentList/ConcurrentDictionary.

In Java this is possible, here a nice little link for you:

https://docs.oracle.com/javase/tutorial/essential/concurrency/collections.html

Since a collection is used within a loop, the collection is used for a certain time. If you add/remove in the function called within the loop, you'll get an error as you're not allowed to without Concurrent.

Answer 5

You may first make a copy of listeners Collection to avoid possible ConcurrentModificationException:

public void fireListener() {
  Listener[] ll = listeners.toArray(new Listener[listeners.size()]);
  for(Listener l : ll) l.someMethod();
}