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.hendr...@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
