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
