John is replacing some of the ExpressionHelper uses (properties and bindings) through https://github.com/openjdk/jfx/pull/1081. It's still single threaded, but I think the new implementation there should be the center point of this discussion.
On Wed, Apr 23, 2025 at 9:41 PM Kevin Rushforth <kevin.rushfo...@oracle.com> wrote: > 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 > > > >
