Detecting threading problems faster

[John Hendrikx]

For detecting long running tasks on the FX thread, there are some other 
options which you can do as a user (but perhaps we can support it 
directly within FX).  I use this kind of code to detect long running 
things on the FX thread:

/**

* Adds a slow event warning whenever an event takes more than 10 msto 
process. Note

* that time spent in nested event loops cannot be properly taken into 
account as time

* spent in nested event loops will be part of the event that triggered 
it giving false

* positives. In order for this time to be accurately reflected, the 
methods to enter

* a nested event loop in this class should be used instead of the ones 
in {@link Platform}.

*

* @paramscene a Scene to which to add the slow event warning detection, 
cannot be null

*/

publicstaticvoidaddSlowEventWarning(Scene scene) {

finalEventDispatcher eventDispatcher = scene.getEventDispatcher();

scene.setEventDispatcher(newEventDispatcher() {

privateScheduledFuture<StackTraceElement[]> future;

@Override

publicEvent dispatchEvent(Event event, EventDispatchChain tail) {

if(future!= null) {

future.cancel(false);

}

longstartTime = System.currentTimeMillis();

TIME_TRACKER.enterNested(startTime); // nesting can happen in two ways, 
an event triggering another event, or when a nested event loop is entered

Event returnedEvent = eventDispatcher.dispatchEvent(event, tail);

longendTime = System.currentTimeMillis();

longtimeSpentInNested = TIME_TRACKER.exitNested(endTime);

if(timeSpentInNested > 10) {

longtotal = endTime - startTime;

LOGGER.warning("Slow Event (self/total: "+ timeSpentInNested + "/"+ 
total + " ms @ level "+ TIME_TRACKER.getCurrentLevel() + "): "+ event);

}

returnreturnedEvent;

}

});

}

--John

On 05/08/2024 17:17, Thiago Milczarek Sayão wrote:
> 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/20240807/ffd19ecf/attachment-0001.htm>