[External] : Re: Eager Evaluation of Invalidation Listeners

Wed Oct 6 07:39:16 UTC 2021

Is it possible to dig in the history of ExpressionHelper a bit further? 
In git it seems limited to 9 years ago, but in JIRA I found this bug report:

     https://bugs.openjdk.java.net/browse/JDK-8119521

It describes an issue where an InvalidationListener is not working 
correctly (according to the reporter) and where #get must be called to 
make it behave as expected.  But it turns out this was already fixed -- 
this specific fix might have been the one that introduced the #get call 
in ExpressionHelper.

On 06/10/2021 04:38, Nir Lisker wrote:
> I would also answer "no" to both points. This is also what the docs say.
>
> So the question is: how likely do we think that changing this behavior will
>> break existing applications?
>>
>
> That's the main question. I tested the JavaFX code with the new behavior
> and some tests break immediately, though a few I've looked at seemed to be
> testing the invalidation listener behavior itself (in their own context). I
> don't know what would break outside of the tests. If we go this route, we
> might want to create PRs to fix the tests before we actually change
> the behavior (in contrast to doing it all in the same PR). I think that
> tests fail in several modules and it might require several people to fix
> these tests depending on their areas of expertise. Then we would need to
> test runtime behavior to see what breaks outside of the tests.
>
> In my own codebase nothing breaks, but it's relatively small.
>
> On the related question, I like the idea of nulling out the current value
>> when a property is bound.
>>
>
> I can pick up from where the older PR stopped. It's a simple change.
>
> On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth <kevin.rushforth at oracle.com>
> wrote:
>
>> Given that the intention of InvalidationListener was to be a light-weight
>> way to listen to properties without causing a binding chain to be
>> revalidated, it does seem reasonable to me that the listener is only fired
>> on the valid --> invalid transition, which is the specified behavior, even
>> if the implementation doesn't currently do that in all cases.
>>
>> The two related questions then are:
>>
>> 1. Should adding an invalidation listener to property do an immediate
>> get(), which will ensure that the property is then valid? This will force
>> an eager evaluation of the property and "arm" the property to fire an
>> invalidation even the next time it is invalidated.
>>
>> 2. Should adding an invalidation listener to a currently invalid property
>> cause the listener to be called (either immediately or the next time the
>> object is invalidated)?
>>
>> I think the ideal answer to both questions is "no", although I share
>> John's concern that changing this behavior for InvalidationListeners could
>> break applications. So the question is: how likely do we think that
>> changing this behavior will break existing applications?
>>
>> I think it's something we can and should consider changing. Unless there
>> are serious concerns, I would support changing this behavior as part of
>> avoiding eager evaluation when using invalidation listeners. It would need
>> to be well tested (of course), and would need a CSR describing the
>> compatibility risk, and should probably get a release note.
>>
>> Any concerns in doing this?
>>
>> On the related question, I like the idea of nulling out the current value
>> when a property is bound.
>>
>> -- Kevin
>>
>>
>> On 9/11/2021 9:41 AM, Nir Lisker wrote:
>>
>> I think that we need your input on this to move it forward.
>>
>> On Fri, Sep 3, 2021 at 7:49 AM Nir Lisker <nlisker at gmail.com> wrote:
>>
>>> so the value field should perhaps be nulled out when bound.
>>>
>>>
>>> There was a PR for something like that in the old repo:
>>> https://github.com/javafxports/openjdk-jfx/pull/110
>>> <https://urldefense.com/v3/__https://github.com/javafxports/openjdk-jfx/pull/110__;!!ACWV5N9M2RV99hQ!bIbtLsC0tg02c9a_lgKnM1Xta2USX8QkKRL4imOUSw8xshJsVquOeulplJR7dfEzQUf6$>
>>>
>>> On Fri, Sep 3, 2021 at 5:35 AM John Hendrikx <hjohn at xs4all.nl> wrote:
>>>
>>>>
>>>>
>>>> On 02/09/2021 11:57, Nir Lisker wrote:
>>>>>     So in order
>>>>>     to make sure that a new interested invalidation listener does not
>>>> miss
>>>>>     the fact that a property was *already* invalid, the easiest
>>>> solution
>>>>>     might have been to revalidate it upon registration of a new
>>>> listener
>>>>>
>>>>>
>>>>> But why does an invalidation listener need to know the past state of
>>>> the
>>>>> property? It is only interested in the valid -> invalid transition. If
>>>>> the property was invalid then the listener (in theory) shouldn't
>>>> receive
>>>>> any events anyway on subsequent invalidations. (I understand that you
>>>>> don't justify this, I'm posing it as a general question.)
>>>>
>>>> Strictly speaking, no, if users are using InvalidationListener correctly
>>>> then this is definitely correct behavior. I'm not really advocating a
>>>> change, and I'd even prefer that it be brought in line with the
>>>> documentation.
>>>>
>>>> I think however that many users are not using it correctly and expect an
>>>> invalidation event always the next time the value changes (and their
>>>> listener will read that value, validating it again), making it act like
>>>> a light-weight ChangeListener. I know that I probably have written code
>>>> that made that assumption, and would in the past not even think twice
>>>> about replacing a change with an invalidation listener or vice versa if
>>>> that happened to be a better fit. Which is sort of what happened as well
>>>> in the bidirectional binding PR, and the issue slipped past the author
>>>> and two reviewers.
>>>>
>>>>> I suggest that we split the problem into 2: one is the case where the
>>>>> property was valid when the listener was attached, and the other is
>>>> when
>>>>> it was invalid.
>>>>> * A valid starting state. In this case attaching a listener shouldn't
>>>>> need to do anything. A subsequent invalidation event will be sent
>>>>> regardless. Currently, it is calling get() redundantly.
>>>>
>>>> True, the call to get is redundant in this case. Ugly too, calling get
>>>> and discarding its result, while the intention is to force the property
>>>> to become valid.
>>>>
>>>>> * An invalid starting state. In this case the documentation says that
>>>>> nothing needs to happen, but get() is called anyway. Here, the
>>>>> difference is that a subsequent invalidation event is sent in one case
>>>>> and not in the other. The only way to advance here is to make a design
>>>>> decision on what should happen, at least that's how I see it.
>>>>
>>>> The docs are even more specific I think, they say no more events will be
>>>> generated until it becomes valid -- it doesn't leave any option open
>>>> that it could generate events if it wanted to.
>>>>
>>>>> As to the implementation of a possible solution, suppose we add the
>>>>> isValid method. Upon attaching an invalidation listener, if the
>>>> property
>>>>> is valid, we can skip the get() call. That solves the valid starting
>>>>> state issue. The question is what to do if the property is not valid.
>>>>>
>>>>> I also noticed an odd design choice in the implementation of
>>>> properties:
>>>>> the value field does not update if the property is bound, instead, the
>>>>> result of the binding is returned and the value field holds an outdated
>>>>> value (until the property is unbound).
>>>>
>>>> Yeah, that might not be a wise decision as that can lead to memory being
>>>> referenced that users might expect to be freed. I didn't see anywhere
>>>> defined what will happen to the value of the property when it is unbound
>>>> again. The current implementation will keep its last value (during the
>>>> unbind it will take the last value and assign it to its own value
>>>> field), so the value field should perhaps be nulled out when bound.
>>>>
>>>> --John
>>>>
>>>
>>
>