[rfc] [icedtea-web] Fix EDT hanging on OpenJDK 11
Jiri Vanek
jvanek at redhat.com
Thu Sep 6 13:11:27 UTC 2018
On 09/05/2018 11:02 PM, Laurent Bourgès wrote:
> Hi,
>
> Here is my first patch against iceadtea-web-1.7 repository.
>
> Build providing the patched netx.jar:
> http://jmmc.fr/~bourgesl/itw/ <http://jmmc.fr/%7Ebourgesl/itw/>
>
> Changelog:
> - tested on OpenJDK11-EA (linux) / win / mac
> - added Swingutils to provide several EDT violation checks and a new ThreadPool to dispatch swing
> events through the Main AppContext (and avoid AWT deadlocks)
> - added lots of debugging in Dialog creation
> - fix lots of EDT violations
>
Hello!
This is awesome work.
One nitpick - autoformatting changes slipped in. About 200 lines. Do you mind to remove them from
this patch?
If not' I'm not going to force you.
But pushing them in as separate changes would be awesome.
>
>
...
>
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, "Loading class: " + mainName);
> +
Is this really necessary as MESAGE_ALL?
> Class<?> mainClass = app.getClassLoader().loadClass(mainName);
>
> Method main = mainClass.getMethod("main", new Class<?>[] { String[].class });
> @@ -567,6 +569,7 @@
>
> main.setAccessible(true);
>
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, "Starting application ...");
Same.
> OutputController.getLogger().log("Invoking main() with args: " + Arrays.toString(args));
> main.invoke(null, new Object[] { args });
>
> @@ -891,7 +894,7 @@
...
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java Wed Sep 05 22:49:43 2018 +0200
> @@ -59,6 +59,7 @@
> import net.sourceforge.jnlp.security.viewer.CertificatePane;
> import net.sourceforge.jnlp.util.ImageResources;
> import net.sourceforge.jnlp.util.logging.OutputController;
> +import net.sourceforge.swing.SwingUtils;
>
> /**
> * This is the control panel for Java. It provides a GUI for modifying the
> @@ -398,6 +399,9 @@
> }
>
> public static void main(String[] args) throws Exception {
> + // setup Swing EDT tracing:
> + SwingUtils.setup();
> +
and
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java Wed Sep 05 22:49:43 2018 +0200
> @@ -47,6 +47,7 @@
>
> import static net.sourceforge.jnlp.runtime.Translator.R;
> import net.sourceforge.jnlp.runtime.html.browser.LinkingBrowser;
> +import net.sourceforge.swing.SwingUtils;
>
> /**
> * This is the main entry point for the JNLP client. The main method parses the
> @@ -95,6 +96,9 @@
> * @param argsIn launching arguments
> */
> public static void main(String[] argsIn) throws UnevenParameterException {
> + // setup Swing EDT tracing:
> + SwingUtils.setup();
> +
> optionParser = new OptionParser(argsIn, OptionsDefinitions.getJavaWsOptions());
>
> if (optionParser.hasOption(OptionsDefinitions.OPTIONS.VERBOSE)) {
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Sep 05 22:49:43 2018 +0200
> @@ -48,9 +48,9 @@
> import javax.net.ssl.SSLSocketFactory;
> import javax.net.ssl.TrustManager;
> import javax.swing.JOptionPane;
> -import javax.swing.JWindow;
> import javax.swing.UIManager;
> import javax.swing.text.html.parser.ParserDelegator;
> +import net.sourceforge.swing.SwingUtils;
>
> import net.sourceforge.jnlp.DefaultLaunchHandler;
> import net.sourceforge.jnlp.GuiLaunchHandler;
> @@ -740,15 +740,11 @@
> headless = true;
> }
> if (!headless) {
> - try {
> - new JWindow().getOwner();
> - } catch (Exception ex) {
> + if (SwingUtils.getOrCreateWindowOwner() == null) {
> headless = true;
> - OutputController.getLogger().log(ex);
> - OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, Translator.R("HEADLESS_MISSCONFIGURED"));
> }
> }
> - } catch (SecurityException ex) {
> + } catch (Exception e) {
Is generic catch safe? getOrCreateWindowOwner do not add another exception.
> } finally {
> headlessChecked = true;
> }
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Wed Sep 05 22:49:43 2018 +0200
> @@ -23,12 +23,12 @@
> import java.security.AccessControlException;
> import java.security.Permission;
>
> -import javax.swing.JWindow;
>
> import net.sourceforge.jnlp.security.SecurityDialogs.AccessType;
> import net.sourceforge.jnlp.services.ServiceUtil;
> import net.sourceforge.jnlp.util.logging.OutputController;
> import net.sourceforge.jnlp.util.WeakList;
> +import net.sourceforge.swing.SwingUtils;
> import sun.awt.AWTSecurityManager;
> import sun.awt.AppContext;
>
> @@ -114,7 +114,11 @@
> // called for it (and not disposed).
>
> if (!JNLPRuntime.isHeadless()) {
> - new JWindow().getOwner();
> + /*
> + should be shared with JNLPRuntime as checkHeadless() does the same
> + or totally useless ?
> + */
> + SwingUtils.getOrCreateWindowOwner();
Fair comment, and tbh, I dont know.
> }
>
> mainAppContext = AppContext.getAppContext();
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/security/SecurityDialog.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialog.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialog.java Wed Sep 05 22:49:43 2018 +0200
Many many formatincg changes. If you wish, I can push the formatting of this class ahead of time.
> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java Mon May 14 17:15:38 2018 +0200
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java Wed Sep 05 22:49:43 2018 +0200
> @@ -247,7 +247,7 @@
> msg.lock.release();
> }
> }
> - /**
> + /**
Blame on ITW, but stil the same.
> * Post a message to the security event queue. This message will be picked
> * up by the security thread and used to show the appropriate security
> * dialog.
...
> - getViwableDialog().addWindowListener(adapter);
> +// getViwableDialog().addWindowListener(adapter);
Why this change?
...
> diff -r bcbef8d7bbd6 netx/net/sourceforge/swing/SwingUtils.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/swing/SwingUtils.java Wed Sep 05 22:49:43 2018 +0200
> @@ -0,0 +1,161 @@
> +// Copyright (C) 2001-2003 Jon A. Maxwell (JAM)
> +//
Can you please use license from icedtea? In ITW it is header like:
/* Copyright (C) 2015 Red Hat, Inc.
This file is part of IcedTea.
IcedTea is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as published by
the Free Software Foundation, version 2.
...
> +// This library is free software; you can redistribute it and/or
> +// modify it under the terms of the GNU Lesser General Public
> +// License as published by the Free Software Foundation; either
> +// version 2.1 of the License, or (at your option) any later version.
> +//
> +// This library is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +// Lesser General Public License for more details.
> +//
> +// You should have received a copy of the GNU Lesser General Public
> +// License along with this library; if not, write to the Free Software
> +// Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> +package net.sourceforge.swing;
> +
> +import java.awt.Window;
> +import java.lang.reflect.InvocationTargetException;
> +import java.util.concurrent.Callable;
> +import java.util.concurrent.ExecutionException;
> +import java.util.concurrent.ExecutorService;
> +import java.util.concurrent.Future;
> +import java.util.concurrent.LinkedBlockingQueue;
> +import java.util.concurrent.ThreadFactory;
> +import java.util.concurrent.ThreadPoolExecutor;
> +import java.util.concurrent.TimeUnit;
> +import java.util.concurrent.atomic.AtomicInteger;
> +import javax.swing.JDialog;
> +import javax.swing.JWindow;
> +import javax.swing.RepaintManager;
> +import javax.swing.SwingUtilities;
> +import net.sourceforge.jnlp.Launcher;
> +import net.sourceforge.jnlp.runtime.Translator;
> +import net.sourceforge.jnlp.util.logging.OutputController;
> +
> +/**
> + * Swing / AWT utility class
> + * @author Laurent Bourgès
> + */
> +public final class SwingUtils {
> +
> + private static final boolean INFO_DIALOG = false;
> + private static final boolean DEBUG_EDT = false;
> + private static final boolean TRACE_EDT = false;
Does any one f those sense to been enabled by ITW verbose mode? Does anyone have sense to be enabled
via -Djava.awt.itw.whatever ?
You are using sout here. This is violating ITW logging bottle neck. However ITW have logging -
unless disabled in itw settngs - bound to console, which is awt. So it can be contra productive.
Thoughts?
> +
> + /** main thread group */
> + /* package */static final ThreadGroup rootGroup = Thread.currentThread().getThreadGroup();
> +
> + private SwingUtils() {
> + // forbidden
> + }
> +
> + public static void setup() {
> + if (DEBUG_EDT) {
> + TracingEventQueue.install();
> +
> + System.out.println("Using ThreadCheckingRepaintManager");
> + RepaintManager.setCurrentManager(new ThreadCheckingRepaintManager());
> + }
> + }
> +
> + public static void checkEDT() {
> + if (!SwingUtilities.isEventDispatchThread()) {
> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, new Exception("EDT violation"));
> + }
> + }
> +
> + public static void info(final JDialog dialog) {
> + if (INFO_DIALOG) {
> + System.out.println("Dialog[" + dialog.getName() + "]"
> + + " in TG [" + Thread.currentThread().getThreadGroup() + "]");
> + }
> + }
> +
> + public static void invokeLater(final Runnable doRun) {
> + EDT_DAEMON_THREAD_POOL.submit(new Runnable() {
> + @Override
> + public void run() {
> + if (TRACE_EDT) {
> + System.out.println("invokeLater in TG [" + Thread.currentThread().getThreadGroup() + "]");
> + }
> + SwingUtilities.invokeLater(doRun);
> + }
> + });
> + }
> +
> + public static void invokeAndWait(final Runnable doRun) throws InterruptedException, InvocationTargetException {
> + Future<?> future = EDT_DAEMON_THREAD_POOL.submit(new Callable<Void>() {
> + @Override
> + public Void call() throws Exception {
> + if (TRACE_EDT) {
> + System.out.println("invokeAndWait in TG [" + Thread.currentThread().getThreadGroup() + "]");
> + }
> + SwingUtilities.invokeAndWait(doRun);
> + return null;
> + }
> + });
> + try {
> + // Wait on Future:
> + future.get();
> + } catch (ExecutionException ee) {
> + if (ee.getCause() instanceof InvocationTargetException) {
> + throw (InvocationTargetException) ee.getCause();
> + }
> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, ee);
> + }
> + }
> +
> + private static final class MainAppContextDaemonThreadFactory implements ThreadFactory {
> +
> + private final AtomicInteger threadNumber = new AtomicInteger(1);
> + private final String namePrefix = "itw-edt-thread-";
> +
> + @Override
> + public Thread newThread(Runnable r) {
> + final Thread t = new Thread(rootGroup, r,
> + namePrefix + threadNumber.getAndIncrement()
> + );
> + if (!t.isDaemon()) {
> + t.setDaemon(true);
> + }
> + if (t.getPriority() != Thread.NORM_PRIORITY) {
> + t.setPriority(Thread.NORM_PRIORITY);
> + }
> + return t;
> + }
> + }
> +
> + /** single thread pool with max 1 live daemon thread */
> + private static final ExecutorService EDT_DAEMON_THREAD_POOL = new ThreadPoolExecutor(0, 1,
> + 60L, TimeUnit.SECONDS,
> + new LinkedBlockingQueue<Runnable>(),
> + new MainAppContextDaemonThreadFactory()
> + );
> +
> + /* shared Window owner */
> + private static Window window = null;
> +
> + public static synchronized Window getOrCreateWindowOwner() {
> + if (window == null) {
> + try {
> + SwingUtils.invokeAndWait(new Runnable() {
> + @Override
> + public void run() {
> + try {
> + window = new JWindow().getOwner();
> + window.setName("getOrCreateWindowOwner");
> + } catch (Exception ex) {
> + OutputController.getLogger().log(ex);
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, Translator.R("HEADLESS_MISSCONFIGURED"));
> + }
> + }
> + });
> + } catch (Exception e) {
> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
> + }
> + }
> + return window;
> + }
> +}
.....> +public final class TracingEventQueue extends EventQueue {
...
> +public final class ThreadCheckingRepaintManager extends RepaintManager {
I do not have preference for those two classes. I have never been so deep in AWT debugging to need
this, But I'm happy to learn the approach. And tehy can go in.
Thanx!
J.
More information about the distro-pkg-dev
mailing list