ExpressionHelper thread-safety

Thu Apr 24 02:25:27 UTC 2025

On 23/04/2025 20:59, Andy Goryachev wrote:
>
> Even though JavaFX explicitly permits creating Nodes and Scenes in a
> thread other than the Application Thread, I think it is still a bad
> idea, and I would strongly suggest against doing so.  The code might
> work - initially - but you will soon discover that it presents a
> constant source of issues, especially after the application is deployed.
>
>  
>
> I would also question the value of such a design.  How many
> milliseconds is being saved by trying to instantiate Nodes in a
> background thread?  If you create only a few objects, there is
> absolutely no benefit (and a huge maintenance burden), but if there
> are too many objects created then maybe one is doing something wrong,
> perhaps instead one should try to create things in batches?
>
The question isn't so much how much milliseconds it saves; the question
is, do you want to burden the FX thread with any multi-millisecond
action, including construction of a big component (like a new Tab full
of controls, or an entire Window)?  The answer to that should be a clear
no; anything that takes more than few milliseconds will mean stuttering
animations as frames will get dropped. 

Constructing components in a background thread is perfectly fine; you
however should NOT connect these components on the background thread to
any properties that may receive FX thread callbacks.  So the proper way
to construct a UI component in the background is:

- Construct large graph on a background thread, but do not connect to
any external/global properties yet; take as much time as you need, the
rest of the UI will keep responding and animating
- Then after the heavy work is done, connect to any external properties
on the FX thread
- When the UI has served its purpose, don't forget to disconnect from
any external properties

This can be largely transparent by using the "when" construct; using
this construct you can make links with the external properties
immediately even on the background thread.  With the proper
when-condition, the actual listeners are added just-in-time, and on the
correct thread (typically, you'd use a condition that tracks whether the
control is part of an active scene graph, like
node->scene->window->isShowing).

--John

>  
>
> So my recommendation would remain the same: please don't.  Always
> access JavaFX objects from the Application Thread.
>
>  
>
> -andy
>
>  
>
>  
>
>  
>
>  
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of Kevin
> Rushforth <kevin.rushforth at oracle.com>
> *Date: *Wednesday, April 23, 2025 at 11:41
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Re: ExpressionHelper thread-safety
>
> This came up most recently in the discussion of
> https://github.com/openjdk/jfx/pull/1697
>
> As noted by you and in that PR, properties are not thread-safe. If two
> threads add a listener concurrently, or if one thread adds a listener
> while and another thread notifies the listeners, it is likely to fail.
>
> So the question is: Is it worth doing something about this? And if so,
> how far do we go?
>
> Making the add/remove listeners operations on ExpressionHelper (and
> related classes?) thread-safe so that listeners could be added or
> removed on any thread concurrently with each other and with the
> operation off firing a listener probably wouldn't be too hard or have
> much downside (the performance impact should be negligible and it is
> unlikely to cause a deadlock).
>
> You still wouldn't be able to modify a property on more than one thread,
> nor control the thread on which listeners are notified (they are
> notified on the thread that mutates the property), so it won't magically
> solve all your threading issues; and you still would need to deal with
> the fact that your listener can be called on a different thread than the
> one which added it.
>
> I'd like to hear from Andy, John, and others as to whether they think
> there is value in providing partial thread-safety for the add/remove
> listener methods of properties.
>
> -- Kevin
>
>
> On 4/23/2025 9:58 AM, 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/54e887ab/attachment-0001.htm>