Thread: Best way to solve this ConcurrentModificationException

Hi.

I have a problem with a concurrent modification exception in a listener-based application. It comes from removing a listener while the listener is active.
Here is a simple example program:

public class YouCantStopTheSignal {
 
	public static void main(String[] args) {
		SignalEmitter se = new SignalEmitter();
		new SignalUser(se);
		se.sendSignal("A");
		se.sendSignal("B");
		se.sendSignal("C");
		se.sendSignal("disconnect"); // boom! -> ConcurrentModificationException
		se.sendSignal("D");
		sg.sendSignal("E");
	}
 
}

public class SignalEmitter {
 
	private final List<SignalListener> listeners;
 
	public SignalEmitter() {
		listeners = new ArrayList<>();
	}
 
	public void addListener(SignalListener l) {
		listeners.add(l);
	}
 
	public void removeListener(SignalListener l) {
		listeners.remove(l);
	}
 
	protected void sendSignal(String signal) {
		for (SignalListener l : listeners) {
			l.send(signal);
		}
	}
 
}

public class SignalUser implements SignalListener {
 
	private final SignalEmitter signalEmitter;
 
	public SignalUser(SignalEmitter se) {
		signalEmitter = se;
		signalEmitter.addListener(this);
	}
 
	public void send(String signal) {
		if (signal.equals("disconnect")) {
			signalEmitter.removeListener(this); // boom! -> ConcurrentModificationException
		} else {
			// doStuff
			System.out.println("In: "+signal);
		}
	}
 
}

I have 2 solutions in mind, but I dont like either of those.

Solution 1:
I add an additional List<SignalListener> to my SignalEmitter. This list will hold SignalListeners that are supposed to be removed:
Changes:

public class SignalEmitter {
 
	private final List<SignalListener> listeners;
	private final List<SignalListener> toBeRemoved;
 
	public SignalEmitter() {
		listeners = new ArrayList<>();
		toBeRemoved = new ArrayList<>();
	}
 
	public void addListener(SignalListener l) {
		listeners.add(l);
	}
 
	public void removeListener(SignalListener l) {
		toBeRemoved.add(l);
	}
 
	protected void sendSignal(String signal) {
		for (SignalListener l : listeners) {
			l.send(signal);
		}
		listeners.removeAll(toBeRemoved);
		toBeRemoved.clear();
	}
 
}

I dont like this solution because it appears to me to be a little bit of overkill for this rather simple task.


Solution 2:
I add another method to my SignalListener. This method will tell the SignalEmitter whether the listener is still interested or not.
Changes:

public class SignalEmitter {
 
	private final List<SignalListener> listeners;
 
	public SignalEmitter() {
		listeners = new ArrayList<>();
	}
 
	public void addListener(SignalListener l) {
		listeners.add(l);
	}
 
	protected void sendSignal(String signal) {
		Iterator<SignalListener> iterator = listeners.iterator();
		while (iterator.hasNext()) {
			SignalListener l = iterator.next();
			l.send(signal);
			if (l.notInterestedAnymore()) {
				iterator.remove();
			}
		}
	}
 
}

public class SignalUser implements SignalListener {
 
	private boolean notInterestedAnymore;
 
	public SignalUser(SignalEmitter se) {
		se.addListener(this);
		notInterestedAnymore = false;
	}
 
	public void send(String signal) {
		if (signal.equals("disconnect")) {
			notInterestedAnymore = true;
		} else {
			// doStuff
			System.out.println("In: "+signal);
		}
	}
 
	public boolean notInterestedAnymore() {
		return notInterestedAnymore;
	}
 
}

I dont like this solution either, mainly because it is clunky and verbose. Also: in my opinion it should not be an attribute of the listener whether it is listening or not.




What do you think? What should I do?
Is there a better way?

Thanks in advance.

Sure, if you really need it.

Exception in thread "main" java.util.ConcurrentModificationException
	at java.util.ArrayList$Itr.checkForComodification(Unknown Source)
	at java.util.ArrayList$Itr.next(Unknown Source)
	at pkg.SignalEmitter.sendSignal(SignalEmitter.java:23)
	at pkg.YouCantStopTheSignal.main(YouCantStopTheSignal.java:14)

Still looking for help. I would really appreciate just some opinions from experienced programmers.

for (SignalListener l : listeners) {
    l.send(signal);
}

Behind the scenes, this creates an iterator. Whilst this iterator is open the .send method is attempting to remove an item from the collection, hence the ConcurrentModificationException.

I think solution #1 is the best (keep a list of objects to be removed), however personally I would have structured this a little closer to #2 with a boolean member variable in the SignalUser class.

Maybe something like this?

import java.util.ArrayList;
import java.util.List;
import java.util.ListIterator;
 
public class SignalEmitter {
 
  private final List<SignalListener> listeners;
  private ListIterator<SignalListener> li;
 
  public SignalEmitter() {
    listeners = new ArrayList<>();
  }
 
  public void addListener(SignalListener l) {
    listeners.add(l);
  }
 
  public void removeListener(SignalListener l) {
    if (li != null) {
      while(li.previous() != l) {
        // Do nothing
      }
      li.remove();
      // Readjust the iterator if necessary
    } else
      listeners.remove(l);
  }
 
  protected void sendSignal(String signal) {
    li = listeners.listIterator();
    while (li.hasNext())
      li.next().send(signal);
    li = null;
  }
 
  public List<SignalListener> getListeners() {
    return listeners;
  }
 
}

It's a little less verbose than Solution 2, though I'm not sure if this accomplishes what you were going for.

Well, I just went with multiple Lists.
For this purpose I wrote this class, maybe somebody else can find use for it:

import java.util.ArrayList;
import java.util.ConcurrentModificationException;
import java.util.Iterator;
import java.util.List;
 
public class BufferedCollection<E> implements Iterable<E> {
 
	/**
	 * Holds data to be iterated over
	 */
	private final List<E> buffer;
	/**
	 * Holds data to be inserted into the main buffer before the next iteration
	 */
	private final List<E> insertBuffer;
	/**
	 * Holds data to be removed from the main buffer or the insertion buffer before the next iteration
	 */
	private final List<E> deleteBuffer;
 
	public BufferedCollection() {
		buffer = new ArrayList<>();
		insertBuffer = new ArrayList<>(3);
		deleteBuffer = new ArrayList<>(3);
	}
 
	/**
	 * Returns the number of elements that will be iterated over the next time an Iterator is created.
	 * 
	 * @return	Number of elements in this collection ignoring buffers
	 */
	public int getCurrentSize() {
		return buffer.size();
	}
 
	/**
	 * Makes a best effort to predict the size of this collection after the next {@link #mergeBuffers()} has been called.
	 * The returned value might not be accurate all the time.
	 * 
	 * @return	Predicted number of elements in this collection and buffers
	 * @see #bufferedInsert(Object)
	 * @see #bufferedDelete(Object)
	 * @see #mergeBuffers()
	 */
	public int getExpectedSize() {
		return (buffer.size() + insertBuffer.size()) - deleteBuffer.size();
	}
 
	/**
	 * The given object will be inserted into this collection after the next call to {@link #mergeBuffers()}.
	 * 
	 * @param obs
	 */
	public void bufferedInsert(E obs) {
		insertBuffer.add(obs);
	}
 
	/**
	 * The given object will be removed from this collection after the next call to {@link #mergeBuffers()}.
	 * <br>
	 * The object will also be removed if it was supposed to be inserted at the same time.
	 * 
	 * @param obs
	 */
	public void bufferedDelete(E obs) {
		deleteBuffer.add(obs);
	}
 
	/**
	 * This method will carry out all pending insertion and deletion events on this collection.
	 * This should not happen while an iteration over this collection is taking place.
	 * Ideally you would want to call this method before an iteration starts.
	 * <br>
	 * If this method is called during an iteration over the elements within this collection a {@link ConcurrentModificationException} will be thrown.
	 * 
	 * @see #bufferedInsert(Object)
	 * @see #bufferedDelete(Object)
	 */
	public void mergeBuffers() {
		buffer.addAll(insertBuffer);
		insertBuffer.clear();
 
		buffer.removeAll(deleteBuffer);
		deleteBuffer.clear();
	}
 
	public Iterator<E> iterator() {
		return buffer.iterator();
	}
 
	public String toString() {
		return buffer.toString();
	}
 
}