<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>I haven't got anything concrete yet, but I did look into
"transactional" properties before for a similar reason.</p>
<p>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:</p>
<p> // define public observables like this:<br>
x.when(finishedUpdating);<br>
y.when(finishedUpdating);<br>
<br>
// In code that must be executed as a block that sets
properties do:<br>
finishedUpdating.set(false);<br>
<br>
// modify x/y<br>
// do other reentrant unsafe stuff<br>
<br>
// finally allow callbacks again:<br>
finishedUpdating.set(true); // this triggers the events<br>
<br>
Of course, this only works with observables, and not with writable
properties, so we need to invent something new here...<br>
</p>
<p>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.<br>
</p>
<p>The whole process of adding a child to a Parent is supposed to be
atomic:</p>
<p>- There should never be a child that is part of two Parents (or
is used as clip, etc)<br>
- There should be no duplicate children in a single Parent<br>
- Child.scene always equals Parent.scene<br>
- etc...<br>
</p>
<p>Yet you can violate some of these by attaching listeners to the
properties/lists it is manipulating because the process is not
atomic.</p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
<p></p>
<p>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:<br>
</p>
<div style="background-color:#ffffff;padding:0px 0px 0px 2px;">
<div
style="color:#000000;background-color:#ffffff;font-family:"Consolas";font-size:11pt;white-space:pre;"><p
style="margin:0;"><span style="color:#3f5fbf;"> * </span><span
style="color:#7f7f9f;"><b></span><span
style="color:#3f5fbf;">Warning:</span><span style="color:#7f7f9f;"></b></span><span
style="color:#3f5fbf;"> This class directly accesses the source list to acquire information about the changes.</span></p><p
style="margin:0;"><span style="color:#3f5fbf;"> * </span><span
style="color:#7f7f9f;"><br></span><span
style="color:#3f5fbf;"> This effectively makes the Change object invalid when another change occurs on the list.</span></p><p
style="margin:0;"><span style="color:#3f5fbf;"> * </span><span
style="color:#7f7f9f;"><br></span><span
style="color:#3f5fbf;"> For this reason it is </span><span
style="color:#7f7f9f;"><b></span><span
style="color:#3f5fbf;">not safe to use this class on a different thread</span><span
style="color:#7f7f9f;"></b></span><span
style="color:#3f5fbf;">.</span></p><p style="margin:0;"><span
style="color:#3f5fbf;"> * </span><span style="color:#7f7f9f;"><br></span><span
style="color:#3f5fbf;"> It also means </span><span
style="color:#7f7f9f;"><b></span><span
style="color:#3f5fbf;">the source list cannot be modified inside the listener</span><span
style="color:#7f7f9f;"></b></span><span
style="color:#3f5fbf;"> since that would invalidate this Change object</span></p><p
style="margin:0;"><span style="color:#3f5fbf;"> * for all subsequent listeners.</span></p></div>
</div>
<p>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:<br>
</p>
<div style="background-color:#ffffff;padding:0px 0px 0px 2px;">
<div
style="color:#000000;background-color:#ffffff;font-family:"Consolas";font-size:11pt;white-space:pre;"><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">protected</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">final</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">void</span><span
style="color:#000000;"> beginChange() {</span></p><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">if</span><span
style="color:#000000;"> (</span><span style="color:#0000c0;">inChangeFire</span><span
style="color:#000000;">) {</span></p><p style="margin:0;"><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">throw</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">new</span><span
style="color:#000000;"> ConcurrentModificationException();</span></p><p
style="margin:0;"><span style="color:#000000;"> }</span></p><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000c0;">changeBuilder</span><span
style="color:#000000;">.beginChange();</span></p><p
style="margin:0;"><span style="color:#000000;"> }</span></p><p
style="margin:0;">
</p><div style="background-color:#ffffff;padding:0px 0px 0px 2px;"><div
style="color:#000000;background-color:#ffffff;font-family:"Consolas";font-size:11pt;white-space:pre;"><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">protected</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">final</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">void</span><span
style="color:#000000;"> fireChange(ListChangeListener.Change<? </span><span
style="color:#0000a0;font-weight:bold;">extends</span><span
style="color:#000000;"> E> change) {</span></p><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000c0;">inChangeFire</span><span
style="color:#000000;"> = </span><span
style="color:#0000a0;font-weight:bold;">true</span><span
style="color:#000000;">;</span></p><p style="margin:0;">
</p><p style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">try</span><span
style="color:#000000;"> {</span></p><p style="margin:0;"><span
style="color:#000000;"> ListListenerHelper.</span><span
style="color:#000000;font-style:italic;">fireValueChangedEvent</span><span
style="color:#000000;">(</span><span style="color:#0000c0;">listenerHelper</span><span
style="color:#000000;">, change);</span></p><p style="margin:0;"><span
style="color:#000000;"> }</span></p><p style="margin:0;"><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">finally</span><span
style="color:#000000;"> {</span></p><p style="margin:0;"><span
style="color:#000000;"> </span><span
style="color:#0000c0;">inChangeFire</span><span
style="color:#000000;"> = </span><span
style="color:#0000a0;font-weight:bold;">false</span><span
style="color:#000000;">;</span></p><p style="margin:0;"><span
style="color:#000000;"> }</span></p><p style="margin:0;"><span
style="color:#000000;"> }</span></p></div></div></div>
</div>
<p>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:<br>
<br>
Transaction tx = TX_THREAD_LOCAL.get();</p>
<p> if (tx == null) {<br>
invalidate();<br>
}<br>
else {<br>
// in transaction, register listener to invalidate when
it ends<br>
tx.subscribe(this::invalidate);<br>
}<br>
<br>
The Transaction class could also somehow be used to support
tracking list/set changes and consolidate these changes.<br>
</p>
<p>--John<br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 23/08/2024 04:31, Michael Strauß
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAJEpuXSn0ac==c1K7syDfO14U+42XTSPHjuy+nVKkmgELu_-ug@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:john.hendrikx@gmail.com"><john.hendrikx@gmail.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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
</pre>
</blockquote>
</blockquote>
</body>
</html>