ExpressionHelper thread-safety
Christopher Schnick
crschnick at xpipe.io
Thu Apr 24 22:25:43 UTC 2025
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?
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?
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/5a9df3d3/attachment-0001.htm>
More information about the openjfx-dev
mailing list