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

[Laurent Bourgès]

Mario,

>     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.
>

It is related to Swing/Awt internals that associate awt.AppContext to one
ThreadGroup:
in ITW, several thread groups are in use (application, security, main ...)
so each thread group can have its own awt.AppContext, EventQueue and
EventDispatcherThread as a consequence.

Swingutilities.invokeLater() -> EventQueue.invokeLater():
 public static void invokeLater(Runnable runnable) {

*        Toolkit.getEventQueue().postEvent(*            new
InvocationEvent(Toolkit.getDefaultToolkit(), runnable));
    }

Toolkit.getEventQueue():
    /* Accessor method for use by AWT package routines. */
    static EventQueue getEventQueue() {

*        return getDefaultToolkit().getSystemEventQueueImpl();*    }

SunToolkit.getSystemEventQueueImpl():
    @Override
    protected EventQueue getSystemEventQueueImpl() {

*        return getSystemEventQueueImplPP();*    }
    static EventQueue getSystemEventQueueImplPP() {

*        return getSystemEventQueueImplPP(AppContext.getAppContext());*    }

AppContext.getAppContext():
    public static AppContext getAppContext() {
        // we are standalone app, return the main app context
        if (numAppContexts.get() == 1 && mainAppContext != null) {
            return mainAppContext;
        }

        AppContext appContext = threadAppContext.get();

        if (null == appContext) {
            appContext = AccessController.doPrivileged(new
PrivilegedAction<AppContext>()
            {
                public AppContext run() {






*                    // Get the current ThreadGroup, and look for it and
its                    // parents in the hash from ThreadGroup to
AppContext --                    // it should be found, because we use
createNewContext()                    // when new AppContext objects are
created.                    ThreadGroup currentThreadGroup =
Thread.currentThread().getThreadGroup();                    ThreadGroup
threadGroup = currentThreadGroup;*
...

*                    AppContext context =
threadGroup2appContext.get(threadGroup);*                    while (context
== null) {
                        threadGroup = threadGroup.getParent();
                        if (threadGroup == null) {
                            // We've got up to the root thread group and
did not find an AppContext
                            // Try to get it from the security manager
                            SecurityManager securityManager =
System.getSecurityManager();
                            if (securityManager != null) {
                                ThreadGroup smThreadGroup =
securityManager.getThreadGroup();
                                if (smThreadGroup != null) {
                                    /*
                                     * If we get this far then it's likely
that
                                     * the ThreadGroup does not actually
belong
                                     * to the applet, so do not cache it.
                                     */
                                    return
threadGroup2appContext.get(smThreadGroup);
                                }
                            }
                            return null;
                        }
                        context = threadGroup2appContext.get(threadGroup);
                    }

                    // In case we did anything in the above while loop, we
add
                    // all the intermediate ThreadGroups to
threadGroup2appContext
                    // so we won't spin again.
                    for (ThreadGroup tg = currentThreadGroup; tg !=
threadGroup; tg = tg.getParent()) {
                        threadGroup2appContext.put(tg, context);
                    }

                    // Now we're done, so we cache the latest key/value
pair.
                    threadAppContext.set(context);

                    return context;
                }
            });
        }

        return appContext;
    }



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

Without this ITW patch, it was hanging, see:
https://bugs.openjdk.java.net/browse/JDK-8204142


>
> >
> >     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.
>

Sorry, I did not catch the problem.
My fix is very minimal and this patch does not fix synchronization issues
already present in ITW.
I tested javaws with the console opened and did not notice any problem.


>
> 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.
>

Could we postpone this point later as I did not want to rewrite too much
the JavaConsole ?
You propose to remove all 'synchronized' keywords in JavaConsole, I am
pretty sure the synchronization overhead does not hurt here...
If you could propose an alternative, I am OK to integrate it.

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180927/6bcc2064/attachment.html>