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();	}   }