<div dir="ltr">John is replacing some of the ExpressionHelper uses (properties and bindings) through <a href="https://github.com/openjdk/jfx/pull/1081">https://github.com/openjdk/jfx/pull/1081</a>. It's still single threaded, but I think the new implementation there should be the center point of this discussion.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 23, 2025 at 9:41 PM Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" target="_blank">kevin.rushforth@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This came up most recently in the discussion of <br>
<a href="https://github.com/openjdk/jfx/pull/1697" rel="noreferrer" target="_blank">https://github.com/openjdk/jfx/pull/1697</a><br>
<br>
As noted by you and in that PR, properties are not thread-safe. If two <br>
threads add a listener concurrently, or if one thread adds a listener <br>
while and another thread notifies the listeners, it is likely to fail.<br>
<br>
So the question is: Is it worth doing something about this? And if so, <br>
how far do we go?<br>
<br>
Making the add/remove listeners operations on ExpressionHelper (and <br>
related classes?) thread-safe so that listeners could be added or <br>
removed on any thread concurrently with each other and with the <br>
operation off firing a listener probably wouldn't be too hard or have <br>
much downside (the performance impact should be negligible and it is <br>
unlikely to cause a deadlock).<br>
<br>
You still wouldn't be able to modify a property on more than one thread, <br>
nor control the thread on which listeners are notified (they are <br>
notified on the thread that mutates the property), so it won't magically <br>
solve all your threading issues; and you still would need to deal with <br>
the fact that your listener can be called on a different thread than the <br>
one which added it.<br>
<br>
I'd like to hear from Andy, John, and others as to whether they think <br>
there is value in providing partial thread-safety for the add/remove <br>
listener methods of properties.<br>
<br>
-- Kevin<br>
<br>
<br>
On 4/23/2025 9:58 AM, Christopher Schnick wrote:<br>
> Hello,<br>
><br>
> I encountered a rare exception where adding listeners to an observable <br>
> value might break when they are added concurrently. This is due to <br>
> ExpressionHelper not being synchronized. I thought about how to fix <br>
> this on my side, but it is very difficult to do. As this is not a <br>
> typical platform thread issue, in my opinion it should be possible to <br>
> add listeners to one observable value from any thread without having <br>
> to think about any potential synchronization issues (which I can't <br>
> solve other than just running everything on one thread).<br>
><br>
> Even worse, due to the size and array being two different variables <br>
> and being incremented unsafely, once such a concurrent modification <br>
> occurs, this invalid state will persist permanently and will cause <br>
> exceptions on any further method call as well. The only solution is to <br>
> restart the application.<br>
><br>
> This is how a stack trace looks like when this occurs:<br>
><br>
> 21:25:38:840 - error: Index 2 out of bounds for length 2<br>
> java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for <br>
> length 2<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)<br>
>     at <br>
> javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)<br>
>     at javafx.beans.binding.StringBinding.bind(StringBinding.java:114)<br>
>     at javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)<br>
>     at <br>
> javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)<br>
>     at <br>
> io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)<br>
>     at <br>
> io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreViewState.lambda$updateContent$1(StoreViewState.java:147)<br>
>     at java.lang.Iterable.forEach(Iterable.java:75)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreViewState.updateContent(StoreViewState.java:147)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreViewState.init(StoreViewState.java:93)<br>
>     at <br>
> io.xpipe.app.core.mode.BaseMode.lambda$onSwitchTo$1(BaseMode.java:109)<br>
>     at io.xpipe.app.util.ThreadHelper.lambda$load$0(ThreadHelper.java:78)<br>
>     at java.lang.Thread.run(Thread.java:1447)<br>
><br>
> 21:25:38:847 - error: Index 3 out of bounds for length 2<br>
> java.lang.ArrayIndexOutOfBoundsException: Index 3 out of bounds for <br>
> length 2<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:248)<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper$Generic.addListener(ExpressionHelper.java:200)<br>
>     at <br>
> com.sun.javafx.binding.ExpressionHelper.addListener(ExpressionHelper.java:65)<br>
>     at <br>
> javafx.beans.binding.ObjectBinding.addListener(ObjectBinding.java:86)<br>
>     at javafx.beans.binding.StringBinding.bind(StringBinding.java:114)<br>
>     at javafx.beans.binding.Bindings$7.<init>(Bindings.java:428)<br>
>     at <br>
> javafx.beans.binding.Bindings.createStringBinding(Bindings.java:426)<br>
>     at <br>
> io.xpipe.app.util.StoreStateFormat.shellEnvironment(StoreStateFormat.java:24)<br>
>     at <br>
> io.xpipe.ext.proc.env.ShellEnvironmentStoreProvider.informationString(ShellEnvironmentStoreProvider.java:155)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreEntryWrapper.update(StoreEntryWrapper.java:228)<br>
>     at <br>
> io.xpipe.app.comp.store.StoreEntryWrapper.lambda$setupListeners$3(StoreEntryWrapper.java:143)<br>
>     at <br>
> io.xpipe.app.util.PlatformThread.lambda$runLaterIfNeeded$0(PlatformThread.java:318)<br>
>     at <br>
> com.sun.javafx.application.PlatformImpl.lambda$runLater$4(PlatformImpl.java:424)<br>
>     at <br>
> com.sun.glass.ui.InvokeLaterDispatcher$Future.run$$$capture(InvokeLaterDispatcher.java:95)<br>
>     at <br>
> com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java)<br>
><br>
> This full log goes up to index 50 out of bounds due to the recurring <br>
> nature of this exception.<br>
><br>
> Looking at the implementation of ExpressionHelper, I don't see any <br>
> harm in just synchronizing the methods, at least from my perspective. <br>
> But I guess that is up to the developers to decide. The only real <br>
> solution I have as an application developer is to perform all <br>
> initialization on one thread or just hope that this error is rare <br>
> enough, both of which aren't great options. So I hope that a potential <br>
> synchronization of the ExpressionHelper methods can be considered.<br>
><br>
> Best<br>
> Christopher Schnick<br>
><br>
<br>
</blockquote></div>