Detecting threading problems faster

[Kevin Rushforth]

> Wouldn't it be better to implement this check in assert to avoid any 
> impact in production?

No. Using an assert in a case like this is an anti-pattern. A call to 
assert in a library such as JavaFX is only appropriate for checking an 
invariant in internal logic. If we are going to go down this route of 
doing a thread check when mutating properties of "live" nodes, we will 
throw the same IllegalStateException that is currently thrown by some 
methods on Stage and Scene.

As for the proposal itself, adding this check is an interesting idea. We 
considered doing this back in the JDK 7 (JavaFX 2) time frame, but 
decided not to pursue it then. I think the idea is worth further 
discussion. I would limit any thread checking to setting the property. 
It would be too restrictive (and largely unnecessary) to prevent reading 
a property from the application thread.

The things to consider would be:

1. What is the performance hit of doing this check on the setting of 
every property?
2. What is the effect on bound properties?
3. How intrusive is it in the code?
4. Should we add a property to enable / disable the thread check, 
possibly a three- or four-valued property (allow|warn|debug?|deny), as 
was recently done in JEP 471 for sun.misc.Unsafe memory access methods. 
If so, what should the default be?

My quick take is that if this can be done in a minimally intrusive 
manner with low overhead, we should consider pursing this. As for 4, my 
preference would be to add a three- or four-valued system property to 
control the check, with "warn" as the default initially, changing the 
default to "disallow" in a subsequent version. This would, of course, 
require a lot of testing.

-- Kevin


On 8/4/2024 8:40 PM, quizynox wrote:
> Hello,
>
> Wouldn't it be better to implement this check in assert to avoid any 
> impact in production?
>
> пн, 5 авг. 2024 г. в 03:30, John Hendrikx <john.hendrikx at gmail.com>:
>
>     Hi list,
>
>     I know of quite some bugs and users that have been bitten by the
>     threading model used by JavaFX.  Basically, anything directly or
>     indirectly linked to an active Scene must be accessed on the FX
>     thread.
>     However, as FX also allows manipulating nodes and properties before
>     they're displayed, there can be no "hard" check everywhere to
>     ensure we
>     are on the FX thread (specifically, in properties).
>
>     Now, I think this situation is annoying, as a simple mistake where a
>     Platform.runLater wrapper was forgotten usually results in programs
>     operating mostly flawlessly, but then fail in mysterious and
>     random and
>     hard to reproduce ways.  The blame is often put on FX as the
>     resulting
>     exceptions will almost never show the user code which was the actual
>     culprit.  It can result in FX being perceived as unstable or buggy.
>
>     So I've been thinking if there isn't something we can do to detect
>     these
>     bugs originating from user code much earlier, similar to the
>     `ConcurrentModificationException` the collection classes do when
>     accessed in nested or concurrent contexts.
>
>     I think it may be possible to have properties check whether
>     they're part
>     of an active scene without too much of an performance impact,
>     possibly
>     even behind a switch. It would work like this:
>
>     Properties involved with Nodes will have an associated bean instance
>     (`getBean`).  This is an object, but we could check here if this
>     instance implements an interface:
>
>           if (getBean() instanceof MayBePartOfSceneGraph x) {
>                 if (x.isPartOfActiveScene() && !isOnFxThread()) {
>                      throw new IllegalStateException("Property must
>     only be
>     used from the FX Application Thread");
>                 }
>           }
>
>     This check could be done on every set of the property, and
>     potentially
>     on every get as well.  It should be relatively cheap, but will expose
>     problematic code patterns at a much earlier stage.  There's a chance
>     that this will "break" some programs that seemed to be behaving
>     correctly as well, so we may want to put it behind a switch until
>     such
>     programs (or libraries) can be fixed.
>
>     What do you all think?
>
>     --John
>
>     (*) Names of methods/interfaces are only used for illustration
>     purposes,
>     we can think of good names if this moves forward.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240805/3edf26bf/attachment-0001.htm>