<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 25/04/2025 00:25, Christopher
Schnick wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e448fe5c-e00b-4665-a548-26d49b1dd14d@xpipe.io">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>The custom wrapper solution to synchronize sounds like a good
idea. I will experiment with that. Would it not also be a
solution to add such a synchronized property wrapper class to
JavaFX itself?<br>
</p>
</blockquote>
<p>Yeah, it definitely could be, as long as it doesn't give the
wrong impression (ie. that such a property will function correctly
in all scenario's). You still could only make changes to such
properties on the FX thread if any controls are registered as
listener, synchronized or not.</p>
<blockquote type="cite"
cite="mid:e448fe5c-e00b-4665-a548-26d49b1dd14d@xpipe.io">
<p> <br>
The reason for why the expression helper replaces itself makes
sense. However, is that still a design goal of modern JavaFX to
implement these optimizations just to make the memory footprint
of instances a bit smaller? <br>
</p>
</blockquote>
<p>A single Node has I think over a hundred properties, and per
property potentially two lists to store listeners in. A
conservative estimate (if we didn't optimize for memory here) is
that this would take 5-10 kB to store per Node. Multiply that
with possible 1000 or more nodes, and it does start to get
somewhat significant. As most properties have no listeners, it
makes sense to delay creating a list for storing listeners. There
are other optimizations in play as well; a lot of properties are
created only when modified or registered on (until that time they
don't need to exist); it also speeds up the FX startup time when
less things needing to be created and loaded to get going.</p>
<p>--John<br>
</p>
<blockquote type="cite"
cite="mid:e448fe5c-e00b-4665-a548-26d49b1dd14d@xpipe.io">
<p> </p>
<div class="moz-cite-prefix">On 25/04/2025 00:14, John Hendrikx
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ea0ec13f-4345-4842-99b5-c3b057a1fcbe@gmail.com">
<meta http-equiv="Content-Type"
content="text/html; charset=UTF-8">
<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>
</blockquote>
</blockquote>
</body>
</html>