[rfc] [icedtea-web] Fix EDT hanging on OpenJDK 11

[Mario Torre]

On 09/26/2018 10:02 PM, Laurent Bourgès wrote:
> Hi Mario,
> 
> Thanks for your feedback, here are my answers:
> 
>     1. Why is EDT_DAEMON_THREAD_POOL needed?
> 
> 
> This is critical on OpenJDK 11 as SequencedEvent processing is buggy
> (still not fixed).
> - This specific Thread pool uses a specific factory to create threads in
> main thread group (main thread).
> 
>     public Thread newThread(Runnable r) {
>             final Thread t = new Thread(MAIN_GROUP, r, ...
> - Such particular thread then calls SwingUtilities.invokeXxx() methods,
> making EDT/AWT use the proper main AppContext & EventQueue.
> 
>     EDT_DAEMON_THREAD_POOL.submit(new Runnable() {
>                 public void run() {
>                     SwingUtilities.invokeLater(doRun);
>                 }
>             });
> 
> This pattern allows to transfer any Swing / AWT invocations from any
> running Thread (in or out the main Thread group) into the proper
> AppContext, like a "priviledged" ITW GUI action.

Yeah, but it's still ending up in the EDT, I'm puzzled as to what is
different here than directly using invokeAndWait/invokeLater.

Also, could you point out to what is exactly broken in SequencedEvent?

Only bug I found was a deadlock mention that was dismissed as invalid:

https://bugs.openjdk.java.net/browse/JDK-8155926

> 
>     2. In the previous code, there was a loop until
>     "!splashScreen.isSplashImageLoaded()", your code is functionally
>     different, other than just pushing things into the EDT, it also
>     refactor this call. I'm not very familiar with the inner intricacies
>     and I thought the previous call was redundant and only necessary due
>     to the cross thread synchronisation, but I rather ask your intentions
>     here that assume anything.
> 
> 
> I agree I simplified that code as I did not catch any reason why it is
> so complicated: I did not figure out any real reason to catch 2 times
> (InterruptedException) in JNLPSplashScreen constructor +
> setSplashImageURL() then loop on: while
> (!splashScreen.isSplashImageLoaded()) => it simply does not make sense
> to me.

Yeah, that's fine by me too, I don't think there's any concrete
difference, even if the code does something different.

>     3. It seems that methods that use "observable" are synchronized.
>     However, the EDT will use "observable" and it's not synchronized. Is
>     this safe?
> 
> 
> Ok, fixed:
>                 synchronized(observable) {
>                     if (observable.hasChanged() ||
> (Boolean.TRUE.equals(force))) {
>                         observable.notifyObservers(force);
>                     }
>                 }

Right, but I don't think this addresses the issue, one is synchronized
on the observable, the other methods are on the JavaConsole, so we
should probably pick one.

By going through the code though, it seems like we could just ensure
that updateModel() always run in the EDT and remove all other
synchronised keywords from the methods, the only user of this method
which can execute outside the EDT is addMessage, but otherwise all other
consumers are executing in the EDT.

Cheers,
Mario

-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898