[rfc] [icedtea-web] Fix EDT hanging on OpenJDK 11
Laurent Bourgès
bourges.laurent at gmail.com
Fri Sep 7 07:38:06 UTC 2018
Jiri,
I propose to improve my SwingUtils to get rid of all SwingUtilities
remaining usages... now or after this first patch.
Its invokeLater/AndWait methods could first check if the thread belongs to
the main thread group => use directly EDT (shortcut), or wrap the event. It
will minimize the thread pool overhead.
I noticed ITW also use EventQueue.invokeLater() that I did not wrap yet.
For consistency I would like having SwingUtils replace all occurences of
SwingUtilities / EventQueue to obtain a safe & simple API.
I may have time during this week end so I could then improve the patch.
What do you prefer ?
Laurent
Le jeu. 6 sept. 2018 à 23:36, Laurent Bourgès <bourges.laurent at gmail.com> a
écrit :
> Jiri,
> Here is an updated patch, fixing all your concerns and I made more
> cleanups and fixed code formatting.
>
> Few answers, below:
>
>>
>> This is awesome work.
>>
>
> Thanks, it was quite tricky to fix EDT !
>
>
>> One nitpick - autoformatting changes slipped in. About 200 lines. Do you
>> mind to remove them from
>> this patch?
>>
>
> Fixed.
>
> > +
>> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
>> "Loading class: " + mainName);
>> > +
>>
>> Is this really necessary as MESAGE_ALL?
>>
>
> Moved in logs, but I liked having it in my console as my app is starting
> slowly (from home @ DSL) due to lots of http queries ... latency is then
> important and I like knowing what's going on, nevermind.
>
>>
>> > 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.
>>
>
> Fixed.
>
> > 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.
>>
>
> Fixed.
>
>
>> > } 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.
>>
>
> I reverted formatting changes.
>
>
>>
>> > 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.
>>
>
> Fixed.
>
>
>> > * 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?
>>
>
> I reverted that but I left my explanation in the code saying that is is
> dead code that never happens as JDialog != SecurityDialog ... so this
> listener is useless.
>
>
>
>> Can you please use license from icedtea?
>
>
> Fixed in the 3 new files.
>
> > +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 ?
>>
>
> Only DEBUG_EDT may be helpful, so I fixed:
> DEBUG_EDT = System.getProperty("icedtea-web.edt.debug",
> "false").equalsIgnoreCase("true");
>
>
>>
>>
>> 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.
>>
>
> I wrapped such calls in trace(String) and explained that it is only for
> TRACING/DEBUG EDT (ITW internals) so logging / console may interfere as it
> is using also EDT (chicken-egg problem) !
>
>
>> 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.
>>
>
> Cheers,
> Laurent
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180907/f320a395/attachment.html>
More information about the distro-pkg-dev
mailing list