<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
<div class="moz-cite-prefix">On 24/04/2025 22:07, Christopher
Schnick wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1baaf4de-8329-472a-b9c2-4bcbc8f760d5@xpipe.io">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hey John,<br>
<br>
Thanks for taking your time on going into the details here.<br>
<br>
About our use case: We are actually not constructing UI in a
background thread, all nodes are initialized and added to the
scene on the platform thread. This is done because previously
instantiating nodes on other threads was unstable for some
controls. So this issue has nothing to do with nodes at all and
is purely focused on the observable value listeners. When
initializing our application, there are some global properties
like the app language property that a lot of other listeners and
bindings depend on as a lot of stuff has to be changed when the
language changes (Not just within nodes). So in practice, we
have 100+ listeners added to these important setting properties.
These listeners are added from various threads as the loading is
parallelized. This loading is to some degree done before the
platform is even started.<br>
</p>
</blockquote>
<br>
<p>Okay, so you're adding listeners in parallel, but not changing
the property at the same time listeners are being added? In other
words, there is never a listener being added/removed while
concurrently listeners are being notified? That should be
relatively safe (if properties were thread safe in that regard :))<br>
</p>
<p>For a short term solution, one thing you can do is to wrap these
global properties in a custom wrapper, delegating all methods to
the original FX class. You then add `synchronized` to all the
add/remove listener methods. The users of these properties won't
know the difference, and ExpressionHelper should no longer get in
a bad state.</p>
<blockquote type="cite"
cite="mid:1baaf4de-8329-472a-b9c2-4bcbc8f760d5@xpipe.io">
<p> <br>
About the listener management: Yes I'm aware that this can be
tricky and I'm doing the best to account for all of that. The GC
problem is always a bit tricky and has led to some issues in the
past. But once you are familiar with it, it's manageable. We are
using a somewhat custom implementation to create nodes, link
them to properties, and control their lifecycle properly for GC
handling for that.<br>
<br>
About the expression helper being replaced, that is unfortunate
(and also a bit weird from my unknowing perspective, at least I
don't see the reason for that). If you are already reworking the
listeners in general, then it would be nice if there was a good
way to synchronize the expressionhelper on something that is
consistent.<br>
</p>
</blockquote>
<p>ExpressionHelper replaces itself depending on the type and count
of listeners on the property; it's an optimization to use as
little memory as possible as most properties have 0 listeners, and
the next most common amount of listeners is 1. For those two
special cases, ExpressionHelper is null (for 0 listeners) or it is
one of the SingleXXX instances (which take less memory than the
variants that have an ArrayList).</p>
<p>The reworking is mostly to fix another problem, and potentially
making listener management synchronized can be done in either
case.<br>
</p>
<blockquote type="cite"
cite="mid:1baaf4de-8329-472a-b9c2-4bcbc8f760d5@xpipe.io">
<p> <br>
You mentioned that there is a case that is hard to fix where a
callback occurs on another thread while the instances are
manipulated on the first thread. At least for us, that shouldn't
be a problem. This issue I reported is only about
adding/removing listeners in different threads. So I will be
following the status of your PR for updates.<br>
</p>
</blockquote>
<p>As long as you're aware that such a fix (or the wrapper I
mentioned above) only gives partial thread safety and may confuse
your own listeners if they were recently added and immediately get
notified... :)</p>
<p>The PR mentioned is not specifically created to solve this
problem; if the team agrees that making listener management
thread-safe is the way forward, it will probably be a separate PR
and ticket. The work involved however is fairly trivial and could
be done before or after the PR mentioned is integrated.<br>
</p>
<p>--John<br>
</p>
<blockquote type="cite"
cite="mid:1baaf4de-8329-472a-b9c2-4bcbc8f760d5@xpipe.io">
<p> <br>
Best<br>
Christopher Schnick<br>
</p>
<div class="moz-cite-prefix">On 24/04/2025 03:56, John Hendrikx
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f0c074e8-176b-4465-a239-2dfb781e361e@gmail.com">
<meta http-equiv="Content-Type"
content="text/html; charset=UTF-8">
<p>Hi,</p>
<p>I don't think adding synchronized in ExpressionHelper is
going to really solve your problem. It will just move it
elsewhere, but feel free to let me know your exact scenario.
For now I will make some assumptions.<br>
</p>
<p>I'm assuming you are constructing UI's in a background
thread, and this UI requires listening to some global
properties, like dark/light mode, or any other configuration
that must dynamically change your UI that's basically global,
or some global modeled state that can be independently used,
even without a UI. It's certainly not an unreasonable
scenario in larger applications that may have a lot of
configuration options -- I've been there myself. I usually
call these "global" models; they're not part of any specific
piece of the user interface. Feel free to let me know your
scenario.</p>
<p>I'm fine with UI's being constructed on background threads;
anything that could potentially take more than a millisecond
SHOULD be done on a background thread, as otherwise animations
will stutter. However, there are several gotcha's with
connecting a UI with global models that expose properties that
you must be aware of:</p>
<p>## Listener Management</p>
<p>Any UI component that listens to global properties must
either:<br>
<br>
a) unregister itself when the UI component is removed or
closed (this can be very difficult to track as FX has no
#dispose method that will be called)<br>
b) use a weak listener (discouraged as this can lead to
phantom call backs of UI's you thought no longer existed until
GC runs)<br>
c) only register the listener when the UI is visible, and
immediately unregister when it becomes invisible (this can be
largely automated with the "when" method of ObservableValue)</p>
<p>Failing to do so means your UI component (including all its
children/parents as they refer to those) will never be garbage
collected as a global property is referring to it.</p>
<p>I highly recommend using the "when" construct here.
Basically, whenever you want to listen to a global property
from a UI component insert a "when" statement:<br>
<br>
globalProperty.when(myComponentIsVisible).subscribe(
... ) or addListener( ... )<br>
<br>
Or: <br>
<br>
uiProperty.bind(globalProperty.when(myComponentIsVisible));<br>
<br>
This results in listeners being registered on the FX thread
just before your UI becomes visible to the user. It also
removes the listeners on the FX thread as soon as the UI
becomes invisible. See the documentation for a good condition
to use with when() for this.<br>
</p>
<p>## Global properties may call listeners at unexpected times!<br>
<br>
When you registered on such a property in a background thread,
realize that as soon as you do, you may get a callback from
the FX thread. At that point in time, your presumed single
threaded code that you are constructing on your isolated
thread is being run by two threads. In other words, you can
get a callback from a global property halfway during
construction while your components may be in some half
constructed state. As FX controls are never safe to use
concurrently (and neither will your listener code be) this can
cause intermittent problems.</p>
<p>All that said, let's say we do want to proceed to make
listener management a little bit safer and prevent
ExpressionHelper from going into a bad state. <br>
</p>
<p>Your proposal to just synchronize the methods in
ExpressionHelper will be insufficient. ExpressionHelper
replaces itself on properties all the time, meaning that
having a single invalidation listener on a property is a
different ExpressionHelper instance then when that same
property has 2 invalidation listeners or say just a single
change listener. This is done by properties like this (from
ObjectPropertyBase):<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:#646464;">@Override</span></p><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">public</span><span
style="color:#000000;"> </span><span
style="color:#0000a0;font-weight:bold;">void</span><span
style="color:#000000;"> addListener(InvalidationListener listener) {</span></p><p
style="margin:0;"><span style="color:#000000;"> </span><span
style="color:#0000c0;background-color:#f0d8a8;">helper</span><span
style="color:#000000;"> = ExpressionHelper.</span><span
style="color:#000000;font-style:italic;">addListener</span><span
style="color:#000000;">(</span><span
style="color:#0000c0;background-color:#d4d4d4;">helper</span><span
style="color:#000000;">, </span><span
style="color:#0000a0;font-weight:bold;">this</span><span
style="color:#000000;">, listener);</span></p><p
style="margin:0;"><span style="color:#000000;"> }</span></p></div>
</div>
<p>As you can see, the actual helper is getting replaced in
certain cases (it "morphs" from one internal type to another
depending on what listener types and counts are registered).
That means that the first call may be dealing with Helper#1,
and the second call may also be dealing with Helper#1
(blocking inside ExpressionHelper on a synchronized block)...
but the first call returns a new Helper, including the new
listener. When then the second call runs that was blocked, it
will replace the Helper again but without knowledge of the
listener that was added by the first call. This happens when
going from a single invalidation listener to two invalidation
listeners -- it's a different helper.<br>
</p>
<p>There are two ways around that; you could synchronize at an
earlier level before calling ExpressionHelper, adding
synchronized to the above method and similar methods, in all
property/bindings and read only property classes (about 20
orso). Another is to synchronize on the property itself
(which is passed as "this" in the above snippet). That still
requires modifying 20 classes though as the "removeListener"
variant does not pass "this" currently, so it would need to be
explicitly passed for those as well to have something to
synchronize on.<br>
</p>
<p>The PR which replaces ExpressionHelper (<a
class="moz-txt-link-freetext"
href="https://github.com/openjdk/jfx/pull/1081"
moz-do-not-send="true">https://github.com/openjdk/jfx/pull/1081</a>)
faces similar issues, but in that PR, "this" is passed already
in all cases, giving it something to synchronize on. If that
PR is integrated, then offering thread safe adding/removal of
listeners for all observable that use the new solution can be
done in one central location.</p>
<p>Perhaps it is worth doing; as Kevin mentioned, within FX
itself we've run into problems with registering listeners that
required quite some changes in many places. A central fix may
be preferable; however it can't and won't be a full fix, as
you still must deal with potential callbacks coming in from
another thread shortly after registering -- a scenario that
most developers will likely not be taking into account while
writing what they presume to be single threaded code...<br>
<br>
--John</p>
<br>
<div class="moz-cite-prefix">On 23/04/2025 18:58, Christopher
Schnick wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f69784b1-6614-47e0-94f1-b5d94f0e46e8@xpipe.io">Hello,
<br>
<br>
I encountered a rare exception where adding listeners to an
observable value might break when they are added concurrently.
This is due to ExpressionHelper not being synchronized. I
thought about how to fix this on my side, but it is very
difficult to do. As this is not a typical platform thread
issue, in my opinion it should be possible to add listeners to
one observable value from any thread without having to think
about any potential synchronization issues (which I can't
solve other than just running everything on one thread). <br>
<br>
Even worse, due to the size and array being two different
variables and being incremented unsafely, once such a
concurrent modification occurs, this invalid state will
persist permanently and will cause exceptions on any further
method call as well. The only solution is to restart the
application. <br>
<br>
This is how a stack trace looks like when this occurs: <br>
<br>
21:25:38:840 - error: Index 2 out of bounds for length 2 <br>
java.lang.ArrayIndexOutOfBoundsException: Index 2 out of
bounds for length 2 <br>
at
com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)<br>
at
com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)<br>
at
com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)<br>
at
javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)
<br>
at
javafx.beans.binding.StringBinding.bind(StringBinding.java:114)
<br>
at
javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)
<br>
at
javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)
<br>
at
io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)<br>
at
io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)<br>
at
io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)<br>
at
io.xpipe.app.comp.store.StoreViewState.lambda$updateContent$1(StoreViewState.java:147)<br>
at java.lang.Iterable.forEach(Iterable.java:75) <br>
at
io.xpipe.app.comp.store.StoreViewState.updateContent(StoreViewState.java:147)<br>
at
io.xpipe.app.comp.store.StoreViewState.init(StoreViewState.java:93)
<br>
at
io.xpipe.app.core.mode.BaseMode.lambda$onSwitchTo$1(BaseMode.java:109)
<br>
at
io.xpipe.app.util.ThreadHelper.lambda$load$0(ThreadHelper.java:78)
<br>
at java.lang.Thread.run(Thread.java:1447) <br>
<br>
21:25:38:847 - error: Index 3 out of bounds for length 2 <br>
java.lang.ArrayIndexOutOfBoundsException: Index 3 out of
bounds for length 2 <br>
at
com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)<br>
at
com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)<br>
at
com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)<br>
at
javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)
<br>
at
javafx.beans.binding.StringBinding.bind(StringBinding.java:114)
<br>
at
javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)
<br>
at
javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)
<br>
at
io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)<br>
at
io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)<br>
at
io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)<br>
at
io.xpipe.app.comp.store.StoreEntryWrapper.lambda$setupListeners$3(StoreEntryWrapper.java:143)<br>
at
io.xpipe.app.util.PlatformThread.lambda$runLaterIfNeeded$0(PlatformThread.java:318)<br>
at
com.sun.javafx.application.PlatformImpl.lambda$runLater$4(PlatformImpl.java:424)<br>
at
com.sun.glass.ui.InvokeLaterDispatcher$Future.run$$$capture(InvokeLaterDispatcher.java:95)<br>
at
com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java)<br>
<br>
This full log goes up to index 50 out of bounds due to the
recurring nature of this exception. <br>
<br>
Looking at the implementation of ExpressionHelper, I don't see
any harm in just synchronizing the methods, at least from my
perspective. But I guess that is up to the developers to
decide. The only real solution I have as an application
developer is to perform all initialization on one thread or
just hope that this error is rare enough, both of which aren't
great options. So I hope that a potential synchronization of
the ExpressionHelper methods can be considered. <br>
<br>
Best <br>
Christopher Schnick <br>
<br>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>