Problems with Observers removing themselves during event

[garethconner]

I'm working an application that controls various mechanical devices via
TCP sockets. In order to inform various objects in the application
when the mechanism state changes, I've been employing the Observer
pattern.

This has been working fine except for a specfic sceneraio. Sometimes
the event generated by the Observable causes Observers to be removed
from further event notifications, for instance if the network
connection is closed or the movement sequence is complete. In such a
case, the Observer asks to be removed from the Observable's listener
list which causes ConcurrentModificationException's. I've reproduced a
short, compilable example, that shows my dilema:

package testing;

import java.util.ArrayList;
import java.util.ListIterator;

public class Test {
static Listener listener1;
static Listener listener2;

/**
* @param args
*/
public static void main(String[] args) {
listener1 = new Listener("Listener #1");
listener2 = new Listener("Listener #2");
EventSource source = new EventSource();

source.addListener(listener1);
source.addListener(listener2);

source.firePropertyChanged();

System.out.println("Done");

}

}

class Listener implements IEventListener{
private String name;
public Listener(String name){
this.name = name;
}

public void propertyChanged(EventSource source) {
source.removeListener(Listener.this);
}

public String getName(){
return name;
}

}

interface IEventListener {
public void propertyChanged(EventSource source);
public String getName();
}

class EventSource {
private ArrayList<IEventListener> listeners;

public EventSource(){
listeners = new ArrayList<IEventListener>();
}

public void firePropertyChanged(){
// for (IEventListener listener : listeners){
// System.out.println("listener " + listener.getName() + ".
Collection size = " + listeners.size());
// listener.propertyChanged(this);
// System.out.println("listener " + listener.getName() + ".
Collection size = " + listeners.size());
// }

// ListIterator<IEventListener> iterator =
listeners.listIterator(listeners.size());
// while(iterator.hasPrevious()){
// IEventListener listener = iterator.previous();
// System.out.println("listener " + listener.getName() + ".
Collection size = " + listeners.size());
// listener.propertyChanged(this);
// System.out.println("listener " + listener.getName() + ".
Collection size = " + listeners.size());
// }

ListIterator<IEventListener> iterator = listeners.listIterator();
while (iterator.hasNext()){
IEventListener listener = iterator.next();
System.out.println("listener " + listener.getName() + ". Collection
size = " + listeners.size());
listener.propertyChanged(this);
System.out.println("listener " + listener.getName() + ". Collection
size = " + listeners.size());
}

}

public void addListener(IEventListener listener){
listeners.add(listener);
}

public void removeListener(IEventListener listener){
listeners.remove(listener);
}

}


In the firePropertyChanged method of the EventSource class, I wrote 3
different iteration loops, just to see if the method/direction of
iteration makes any difference. It does make a difference, because
using the for..in loop or the forward iterator doesn't generate an
exception, but the second listener is never notified. The problem
makes sense, since the Oberservers are causing the ArrayList of
Observable to be altered whilst in the middle of iteration. However,
I'm not sure what the best design solution is.

My first instinct is to dump a copy of the ArrayList into an Array just
prior to iterating in the firePropertyChanged method. My concern is
will whether this will scale well when thousands of events are being
generated per second. There may not be an alternative choice, but any
advice/confirmation would be appreciated.

Best regards,
Gareth

 

[Dan Andrews]

I'm working an application that controls various mechanical devices via
TCP sockets. In order to inform various objects in the application
when the mechanism state changes, I've been employing the Observer
pattern.

This has been working fine except for a specfic sceneraio. Sometimes
the event generated by the Observable causes Observers to be removed
from further event notifications, for instance if the network
connection is closed or the movement sequence is complete. In such a
case, the Observer asks to be removed from the Observable's listener
list which causes ConcurrentModificationException's. I've reproduced a
short, compilable example, that shows my dilema:

[snip]

Hi Gareth,

Without looking at your code in great detail, I'd suggest you try to
copy the pattern shown in the java.swing.event.EventListenerList. That
is when you add and remove your listener then these methods are
synchronized as in the EventListenerList. You will also note that a copy
of the listenerList is made at the ends of the add and remove methods.
Hope that helps.

Cheers,

Dan Andrews
- - - - - - - - - - - - - - - - - - - - - - - - -
Ansir Development Limited http://www.ansir.ca
- - - - - - - - - - - - - - - - - - - - - - - - -

 

[garethconner]

Hi Gareth,

Without looking at your code in great detail, I'd suggest you try to
copy the pattern shown in the java.swing.event.EventListenerList. That
is when you add and remove your listener then these methods are
synchronized as in the EventListenerList. You will also note that a copy
of the listenerList is made at the ends of the add and remove methods.
Hope that helps.

Cheers,

Dan Andrews
- - - - - - - - - - - - - - - - - - - - - - - - -
Ansir Development Limitedhttp://www.ansir.ca
- - - - - - - - - - - - - - - - - - - - - - - - -

Hi Dan,

Perfect, thanks for the suggestion. Everything works now.

Thanks for the help,
Gareth

 

[Chris Uppal]

My first instinct is to dump a copy of the ArrayList into an Array just
prior to iterating in the firePropertyChanged method. My concern is
will whether this will scale well when thousands of events are being
generated per second. There may not be an alternative choice, but any
advice/confirmation would be appreciated.

Taking a copy of the list of observers before processing is simple and
effective. It's probably also pretty quick in that the copy time will be
amortised over all the observers; so any other scheme which avoids the copy
will have to use /very/ little extra processing time per observer to beat that.

However, a few other possibilities which might be worth playing with.

1) Require each observer to indicate (as part of the notification/response)
whether it has finished observing. If it has then your main notification loop
can use ListIterator.remove() safely.

2) "Freeze" the list during the notification loop, and store up any "please
remove me" requests until after it has completed, /then/ remove the unwanted
observers.

3) Create your own List implementation which /can/ safely support items being
removed during traversal.

4) Instead of removing observers from the list, replace them (using List.set())
with a dummy observer. For ArrayList, at least, that should count as an unsafe
modification to the list even if it is iterated over. You may want to have a
second pass to remove the dummies, in which case this is a variant on (2), but
it's probably easier just to filter out dummies as you traverse the list so any
dummy may hang around until the next notification event.

5) Maintain a "shadow" copy of the list. I.e. instead of making copy each time
you have to notify everyone, keep the copy around for next time. If, in the
interim, someone adds or removes an observer then just discard your shadow copy
(which you may be in the middle of iterating over) and recreate it next time
you need it.

But, I repeat that just taking a copy is going to be hard to beat.

-- chris

 

[garethconner]

But, I repeat that just taking a copy is going to be hard to beat.

-- chris

Chris,

Thanks very much for your detailed alternatives, I really appreciate
your thoughts. Making a copy of the list within the synchronized
add/removeListener methods, as is done in EventListenerList, so far
seems to work well. This also has the benefit of only copying the list
during adds/removes which do not happen as frequently as plain-jane
iterations.

Thanks again for the alternatives, I'll keep those in my notes for
future troubleshooting if I find a bottleneck in the current
implementation.

Best regards,
Gareth

 

[Tom Hawtin]

Thanks very much for your detailed alternatives, I really appreciate
your thoughts. Making a copy of the list within the synchronized
add/removeListener methods, as is done in EventListenerList, so far
seems to work well. This also has the benefit of only copying the list
during adds/removes which do not happen as frequently as plain-jane
iterations.

You would need to add synchronized to the 'get' methods as well.
EventListenerList only provides 'a degree of thread-safety', which is to
say that it is not thread-safe.

A lot of uses of listeners don't make any sense in a multi-threaded
environment (or where listeners mutate their source). For instance in
Swing, DocumentEvent provides offset and length of the modified area.
Modification of the document in another thread or by a listener may
result in unpredictable (although in some, unuseful, sense
'thread-safe') behaviour.

Tom Hawtin