<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<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></p>
<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">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>
</body>
</html>