ExpressionHelper thread-safety

[John Hendrikx]

On 25/04/2025 00:25, Christopher Schnick wrote:
>
> 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?
>
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.

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

--John

> On 25/04/2025 00:14, John Hendrikx wrote:
>>
>> On 24/04/2025 22:07, Christopher Schnick wrote:
>>>
>>> Hey John,
>>>
>>> Thanks for taking your time on going into the details here.
>>>
>>> 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.
>>>
>>
>> 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 :))
>>
>> 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.
>>
>>>
>>> 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.
>>>
>>> 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.
>>>
>> 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).
>>
>> The reworking is mostly to fix another problem, and potentially
>> making listener management synchronized can be done in either case.
>>
>>>
>>> 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.
>>>
>> 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... :)
>>
>> 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.
>>
>> --John
>>
>>>
>>> Best
>>> Christopher Schnick
>>>
>>> On 24/04/2025 03:56, John Hendrikx wrote:
>>>>
>>>> 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/20250425/02d44426/attachment-0001.htm>