Listeners behavior discussion

[John Hendrikx]

Hi all,

I'm looking into ExpressionHelper as part of my evaluation of adding 
Cleaner's to clean up weak listeners stubs a bit more eagerly.

I would like to hear your thoughts on the following:

ExpressionHelper has a locking mechanism so that when an event occurs, 
it is fired to the listeners which were active at the moment the event 
was triggered. This is achieved by making a copy of the listeners *if* 
the list is modified while an event emission is in progress. This uses a 
simple boolean for the lock ("locked").

It is however conceivable (although perhaps not good practice) to change 
the value again while the event emission is occurring. An easy example I 
can think of is that when a text field is modified, a ChangeListener may 
change the value to uppercase if the field should only contain uppercase 
characters.  This will trigger a nested event emission for the same 
property.  JavaFX does not prohibit this (and probably can't anymore for 
backwards compatibility reasons).  There are several problems with this:

1) The locking mechanism does not take nesting into account, therefore 
when the 2nd nested emission completes, the listener lists are unlocked, 
and the first event emission will continue with the lists unlocked. If 
there is any modification after the 2nd emission completes, listeners 
may be skipped, notified twice or an IndexOutOfBoundsException may occur.

2) The events received by the listeners are inconsistent depending on 
where you are in relation to the listener that made the nested change.  
Let's say there are three listeners, A, B and C in that order.  Listener 
B will change any uneven number to the next higher even number.  The 
events seen by the three listeners when the value changes from 2 to 3 are:

A: 2 -> 3; 3 -> 4
B: 2 -> 3; 3 -> 4
C: 3 -> 4; 2 -> 4

IMHO, we should strive to have the exact same events for all three 
listeners: 2 -> 3; 3 -> 4 or prohibit this with an exception.

I think it is possible to change this so all listeners receive the exact 
same events; this would be achieved by delaying the 2nd event emission 
until the first has finished.

I think the specification for how invalidation/change listener event 
notification works should perhaps be documented somewhere. I think it 
should be:

When an event occurs, the currently list of registered listeners will 
receive this event in the order they were registered, regardless of 
whether the list is modified during the event emission. If during 
emission, another event occurs for the same property, it will be queued 
and emitted immediately after the current emission completes. This 
ensures all registered listeners receive the same events irrespective of 
registration order.  Event emission is serialized, that is to say, no 
two emissions for the same property will ever run concurrently.

=====

Implementation ideas

If we can all agree on the above specification, I think an 
implementation can be created that never needs to copy a listener list.  
The main trick to get this correct is in how events are emitted. First, 
when a nested emission occurs (easily detected by the fact that the list 
is locked), the change for the nested emission is buffered.  The nested 
emission is then triggered as soon as the first finishes (when the first 
emission is about to unlock, it checks for a nested emission, and does 
another emission if one is present, and so on).  If there are multiple 
emissions after an emission finishes, they probably can and should be 
rolled into a single one.  Note that nested emissions can easily go 
wrong (two listeners that change values in conflicting ways) and cause 
an infinite loop -- we're not looking to prevent these kinds of bugs 
here, although we could set a limit before "giving up".

Secondly, while the lists are locked, any additions or removals to the 
list should be buffered. Just before the list is unlocked (and before a 
second emission occurs), all changes are applied. This has the 
additional bonus that any mass changes to the listener list (for example 
when a large number of stubs were determined to require clean up) can 
use mass change functions like #removeAll and #addAll, potentially 
saving a lot of copying of the listener list, on top of the copy that is 
already avoided due to the lock.

The fireValueChangedEvent method should be synchronized, so only the 
same thread could potentially trigger a nested emission.  I think we 
should steer clear from allowing multiple threads to emit events for the 
same property concurrently as this will make the events received 
unpredictable and probably even incorrect (the last change received may 
not reflect the actual current value!).

Thanks for reading, I look forward to hearing your thoughts!

--John