<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>In both cases only a partial fix can be applied that can ensure
that at a minimum the listener management doesn't get into a bad
state. The issue of what happens when a callback occurs on a
second thread while instances are being manipulated on the first
thread will remain.<br>
</p>
<p>The partial fix would involve synchronizing on the property,
which can be done in either all properties themselves, or in the
helper (as they are passed the property reference). For
ExpressionHelper, removeListener currently doesn't pass the
property reference, but it could be added. For the new PR, this
is already the case.</p>
<p>--John<br>
</p>
<div class="moz-cite-prefix">On 23/04/2025 20:54, Nir Lisker wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CA+0ynh_1qfMPoEG_qeo-Q2+43=3ix3v0mh=RVajKLc_rBS8ydw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">John is replacing some of the ExpressionHelper uses
(properties and bindings) through <a
href="https://github.com/openjdk/jfx/pull/1081"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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>
</blockquote>
</body>
</html>