[jfx19] RFR: 8290331 Binding value left null when immediately revalidated in invalidation listener [v2]

[John Hendrikx]

I think the issue in JDK-8206449 might not be entirely resolved.

Even though `value` is now cleared in `ObjectBinding`, I noticed that 
`ExpressionHelper` also maintains the current value when there are 
change listeners registered. Unfortunately, it forgets to clear this 
when all change listeners are removed but there are still sufficient 
invalidation listeners registered for it to need the Generic 
ExpressionHelper implementation.

I think an overhaul of ExpressionHelper might be warranted:

- Doesn't clear current value when it should
- The locking done makes a new copy of the listener lists each time if 
there are multiple changes to these lists during an event emission (this 
can happen easily when many listener stubs have gone out of scope)
- Change event emission should probably not be dependent on the order of 
listeners, see https://bugs.openjdk.org/browse/JDK-8290310
- Further optimizations are possible:
    - Better remove performance -- although there is no JDK collection 
class that has fast remove(T) while allowing duplicates, it isn't hard 
to construct a specialized class that has fast `addLast`, fast 
`remove(T)` and fast iteration
    - Single invalidation listener could just use the helper field 
directly (instanceof check can determine if it is an invalidation 
listener or a helper instance); this can save quite some memory as the 
fast majority of properties/bindings may never have more than 1 listener
    - Single change maybe as well (requires current value to be passed 
into #fireValueChangedEvent, which is how most other ExpressionHelpers, 
except the base ExpressionHelper,  are done) -- note that many, if not 
all, users of ExpressionHelper often also have a copy of the current 
value maintained separately...
    - Small listener lists could use a single array (using instanceof to 
distinguish between invalidation/change)

I also find that there is a test for ExpressionHelper, testing a ton of 
edge cases, but I think this class shouldn't be tested at all.  Instead, 
a test should be there to test the behavior of classes implementing 
Observable and ObservableValue, regardless of whether they use 
ExpressionHelper internally. Such a test would be reusable, regardless 
of how these classes manage their listeners, or if some decide to not 
use `ExpressionHelper`.

A good JUnit test should I think be constructed first, validating the 
current behavior based on classes implementing Observable and 
ObservableValue; these should be thorough enough to cover most (if not 
all) of the edge cases now tested in ExpressionHelperTest.

With any luck, the test can be written in such a way that it can be 
parameterized on the type of class implementing ObservableValue, 
allowing a single test to test behavior of all *Property classes and all 
*Binding classes.

--John

On 15/07/2022 16:09, Nir Lisker wrote:
> On Fri, 15 Jul 2022 07:19:19 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>
>>> I introduced a bug with the fluent bindings PR which affects all ObjectBindings.
>>>
>>> This is the code that fails:
>>>
>>>          SimpleObjectProperty<Boolean> condition = new SimpleObjectProperty<>(true);
>>>          ObservableValue<String> binding = condition.map(Object::toString);
>>>
>>>          binding.addListener(o -> { binding.getValue(); });
>>>
>>>          condition.set(false);
>>>
>>>          assertEquals(false, binding.getValue());  // returns null (!)
>>>
>>> This PR fixes this problem and adds a test case to cover it.
>> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Change explanatory comment to block comment
> Alright, I closed [JDK-8206449](https://bugs.openjdk.org/browse/JDK-8206449) as duplicated of [JDK-8274771](https://bugs.openjdk.org/browse/JDK-8274771).
>
> -------------
>
> PR: https://git.openjdk.org/jfx/pull/829