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
