ExpressionHelper thread-safety

[John Hendrikx]

Hi,

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.

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.

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:

## Listener Management

Any UI component that listens to global properties must either:

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)
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)
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)

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.

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:

       globalProperty.when(myComponentIsVisible).subscribe( ... ) or
addListener( ... )

Or:

       uiProperty.bind(globalProperty.when(myComponentIsVisible));

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.

## Global properties may call listeners at unexpected times!

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.

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. 

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):

@Override

publicvoidaddListener(InvalidationListener listener) {

helper= ExpressionHelper.addListener(helper, this, listener);

}

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.

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.

The PR which replaces
ExpressionHelper (https://github.com/openjdk/jfx/pull/1081) 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.

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

--John


On 23/04/2025 18:58, Christopher Schnick wrote:
> Hello,
>
> 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).
>
> 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.
>
> This is how a stack trace looks like when this occurs:
>
> 21:25:38:840 - error: Index 2 out of bounds for length 2
> java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for
> length 2
>     at
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)
>     at
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)
>     at
> com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)
>     at
> javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)
>     at javafx.beans.binding.StringBinding.bind(StringBinding.java:114)
>     at javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)
>     at
> javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)
>     at
> io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)
>     at
> io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)
>     at
> io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)
>     at
> io.xpipe.app.comp.store.StoreViewState.lambda$updateContent$1(StoreViewState.java:147)
>     at java.lang.Iterable.forEach(Iterable.java:75)
>     at
> io.xpipe.app.comp.store.StoreViewState.updateContent(StoreViewState.java:147)
>     at
> io.xpipe.app.comp.store.StoreViewState.init(StoreViewState.java:93)
>     at
> io.xpipe.app.core.mode.BaseMode.lambda$onSwitchTo$1(BaseMode.java:109)
>     at io.xpipe.app.util.ThreadHelper.lambda$load$0(ThreadHelper.java:78)
>     at java.lang.Thread.run(Thread.java:1447)
>
> 21:25:38:847 - error: Index 3 out of bounds for length 2
> java.lang.ArrayIndexOutOfBoundsException: Index 3 out of bounds for
> length 2
>     at
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)
>     at
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)
>     at
> com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)
>     at
> javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)
>     at javafx.beans.binding.StringBinding.bind(StringBinding.java:114)
>     at javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)
>     at
> javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)
>     at
> io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)
>     at
> io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)
>     at
> io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)
>     at
> io.xpipe.app.comp.store.StoreEntryWrapper.lambda$setupListeners$3(StoreEntryWrapper.java:143)
>     at
> io.xpipe.app.util.PlatformThread.lambda$runLaterIfNeeded$0(PlatformThread.java:318)
>     at
> com.sun.javafx.application.PlatformImpl.lambda$runLater$4(PlatformImpl.java:424)
>     at
> com.sun.glass.ui.InvokeLaterDispatcher$Future.run$$$capture(InvokeLaterDispatcher.java:95)
>     at
> com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java)
>
> This full log goes up to index 50 out of bounds due to the recurring
> nature of this exception.
>
> 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.
>
> Best
> Christopher Schnick
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20250424/dce500a4/attachment-0001.htm>