Accidentally reproduced NPE in synchronizeNodes in combination with enterNestedEventLoop

[John Hendrikx]

I haven't got anything concrete yet, but I did look into "transactional" 
properties before for a similar reason.

The "when" fluent method on ObservableValue does something similar.  
When its condition is false, it doesn't communicate changes downstream, 
but when it is set to true, it will do so immediately (if there was a 
change).  In other words, you could do this:

        // define public observables like this:
        x.when(finishedUpdating);
        y.when(finishedUpdating);

        // In code that must be executed as a block that sets properties do:
        finishedUpdating.set(false);

        // modify x/y
        // do other reentrant unsafe stuff

        // finally allow callbacks again:
        finishedUpdating.set(true);  // this triggers the events

Of course, this only works with observables, and not with writable 
properties, so we need to invent something new here...

I also now don't think other solutions are going to work for the 
children list problem.  Setting "startIdx" after each child update would 
leave updatePeer potentially with children that don't have their Scene 
or Parent set correctly yet... the same goes for de-optimizing 
updatePeer and just letting it sync the entire children list; it would 
encounter children that have no parent or scene set yet if it is 
triggered halfway during the children list update.

The whole process of adding a child to a Parent is supposed to be atomic:

- There should never be a child that is part of two Parents (or is used 
as clip, etc)
- There should be no duplicate children in a single Parent
- Child.scene always equals Parent.scene
- etc...

Yet you can violate some of these by attaching listeners to the 
properties/lists it is manipulating because the process is not atomic.

> Of course, the implementation will be challenging. We'd need to keep
> track of all modifications, and then aggregate those modifications
> into a single event. In this particular example, the two "add" and
> "remove" events would probably be consolidated into a "permutation"
> event.

Yeah, for ObservableLists the problem will be harder then for 
properties.  Properties can track an old value to compare with a bit 
easier.  Keeping a copy of a large list to calculate differences with is 
a bit more annoying.  However, Lists are documented to not allow further 
changes:

* <b>Warning:</b>This class directly accesses the source list to acquire 
information about the changes.

* <br>This effectively makes the Change object invalid when another 
change occurs on the list.

* <br>For this reason it is <b>not safe to use this class on a different 
thread</b>.

* <br>It also means <b>the source list cannot be modified inside the 
listener</b>since that would invalidate this Change object

* for all subsequent listeners.

This means that for Lists we could get away with simply signalling this 
problem with a "ConcurrentModificationException". I added such an 
exception in ObservableListBase and it is effective at stopping the problem:

protectedfinalvoidbeginChange() {

if(inChangeFire) {

thrownewConcurrentModificationException();

}

changeBuilder.beginChange();

}

protectedfinalvoidfireChange(ListChangeListener.Change<? extendsE> change) {

inChangeFire= true;

try{

ListListenerHelper.fireValueChangedEvent(listenerHelper, change);

}

finally{

inChangeFire= false;

}

}

For property classes, I like the idea of a thread scoped type object -- 
the properties are after all being manipulated on a single thread, and a 
thread local check to see if a transactional scope is active could be 
relatively cheap to built into existing properties.  Something like:

       Transaction tx = TX_THREAD_LOCAL.get();

       if (tx == null) {
            invalidate();
       }
       else {
           // in transaction, register listener to invalidate when it ends
           tx.subscribe(this::invalidate);
       }

The Transaction class could also somehow be used to support tracking 
list/set changes and consolidate these changes.

--John


On 23/08/2024 04:31, Michael Strauß wrote:
> That seems to be a tough one.
>
> Delaying the invocation of listeners sounds interesting, as it might
> allow using a pattern like the following:
>
>      childrenTriggerPermutation = true;
>
>      try (var scope = new DelayedEventScope(children)) {
>          children.remove(node);
>          children.add(node);
>      } finally {
>          childrenTriggerPermutation = false;
>      }
>
> The semantics would be that the property implementation will still
> receive notifications with their invalidated() method as the property
> is being modified, but events will only be fired at the end of the
> scope.
> List properties will need a new listChanged() method to allow for the
> same pattern of overriding the method instead of adding a change
> listener.
>
> Of course, the implementation will be challenging. We'd need to keep
> track of all modifications, and then aggregate those modifications
> into a single event. In this particular example, the two "add" and
> "remove" events would probably be consolidated into a "permutation"
> event.
>
> In general, delayed notification scopes for properties could also be
> very useful for application developers.
>
>
> On Thu, Aug 22, 2024 at 9:59 AM John Hendrikx<john.hendrikx at gmail.com>  wrote:
>> I think I figured out the reason why this fails.  The root cause lies in a misconception I've seen in a lot of FX code.
>>
>> JavaFX uses a single event thread model, which ensures all structures are only ever accessed by a single thread.  This frees FX from having to do synchronization on almost every modification you make to properties or the scene graph.
>>
>> However, in many areas it makes the assumption that such code will always run sequentially to completion without interruption, and uses instance fields judiciously to communicate things to deeper nested code or to code further down the line.  But code using instance fields in this way is not safe to re-enter (it is not reentrant-safe) without precautions -- sharing instance fields in this way safely can easily get as complicated as writing multi-threaded code.
>>
>> A simple example that I saw in Parent's toFront code:
>>
>> childrenTriggerPermutation = true;
>>
>> try {
>>
>> children.remove(node);
>>
>> children.add(node);
>>
>> } finally {
>>
>> childrenTriggerPermutation = false;
>>
>> }
>>
>> The above code uses an instance field "childrenTriggerPermutation" to activate an optimization. The optimization will assume that the children are only re-arranged, and no new ones were added or removed.  However, "children" is an ObservableList, which means the user can register listeners on it, which do who knows what.  If such a listener modifies the children list in another way then the code is entered again, but the "childrenTriggerPermutation" optimization will still be enabled causing it to not notice the change the user did.
>>
>> This problem is similar to the ChangeListener old value bug.  When within a change listener you do another change (and so the same code is called **deeper** in the same stack), downstream change listeners will not receive the correct old values because the code is insufficiently reentrant-safe.  ExpressionHelper **tries** to mitigate some of these issues (for cases where listeners are added/removed reentrantly) by making copies of the listener list, but it does not handle this case.
>>
>> Similarly, the bug I encountered in my original post is also such an issue.  While processing the children list changes, several **properties** are being manipulated.  Being properties, these can have listeners of their own that could trigger further modifications and, in complex enough programs, they may even re-enter the same class's code that is sharing instance fields in an unsafe way.  And that's exactly what is happening:
>>
>> 1. The children list change processing is registering the offset of the first changed child in the children list (called "startIdx") as an instance field -- this field is used as an optimization for updatePeer (so it doesn't have to check/copy all children).  It assumes the processing always finishes completely and it will get to the point where it sets "startIdx" but...
>>
>> 2. Before it sets "startIdx" but after the children list is already modified, it modifies several properties.  Being properties, these can have listeners, and as such this can trigger a cascade of further calls in complicated applications.
>>
>> 3. In this case, the cascade of calls included an "enterNestedEventLoop".  Pulses (and things like Platform#runLater) can be handled on such a nested loop, and FX decides that now is as good a time as any to handle a new pulse.
>>
>> 4. The pulse triggers updatePeer calls, among which is the Parent that is still (higher in the stack) midway its children list processing code.
>>
>> 5. The updatePeer code looks at "startIdx", the shared instance field that Parent uses for its optimizations.  This field is NOT modified yet.  The field indicates the first child that was modified, and the field is normally set to "children.size()" when there are no changes.  That's also the case in this case still, and so updatePeer updates nothing at all.  An assertion later in this code then checks if children.size() == peer.children.size() which fails... a stack trace is thrown, and synchronizeSceneNodes() blows up with infinite NPE's.
>>
>> I'm not entirely sure yet how to resolve this, and if it should be.
>>
>> Perhaps the safest way would be to undo some of the optimizations/assumptions, and perhaps reoptimize them if there's a pressing need.
>>
>> Another option would be to somehow delay listener callbacks until the code in Parent is in a safe state.
>>
>> The option I like the least is to introduce yet another instance flag ("processingListChange") and throwing an early exception if other code is entered that doesn't expect it...
>>
>> --John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240823/ae670a18/attachment-0001.htm>