ExpressionHelper thread-safety

Wed Apr 23 19:04:55 UTC 2025

The javafx.beans package for me at least isn't necessarily bound to 
JavaFX nodes. In our case, we use observable values for more than just 
for JavaFX nodes. They are useful on their own. Obviously in most cases 
they are used in relation to nodes, but in our case we instantiate 
properties and add listeners (across multiple threads) before the 
Platform is even started.

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?
>
> 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/20250423/ac979dd9/attachment-0001.htm>