Detecting threading problems faster

[Thiago Milczarek Sayão]

Hi,

Interesting idea. We have this problem specially when Junior developers
touch the code.

The other way around would be nice too - if some I/O task executes on the
FX thread.

This can make the OS think the application hanged and offer to kill it,
since it won't respond to "pings". And I/O tasks processing time may vary
between installations. Also causes "white screens" since it blocks painting.

-- Thiago.

Em seg., 5 de ago. de 2024 às 11:59, Kevin Rushforth <
kevin.rushforth at oracle.com> escreveu:

> 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/6de3aa2a/attachment-0001.htm>