Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: wtk
    • Labels:
      None

      Description

      Greg, hello;

      in this thread:
      http://mail-archives.apache.org/mod_mbox/pivot-user/201001.mbox/%3C4B5E581D.2080604@hms.harvard.edu%3E

      your final word is:
      "Sorry, it is not possible"

      but I know that you know that it is possible

      the reason I need this is same as Martin here:
      http://netbeans.org/bugzilla/show_bug.cgi?id=90590

      namely: do some cleanup after shutdown was requested and confirmed:
      org.apache.pivot.wtk.Application
      public boolean shutdown(boolean optional) throws Exception;

      since you call shutdown(boolean optional) from EDT I need to block it, using this approach:
      http://bugs.sun.com/view_bug.do?bug_id=6424157

      wich "almost works", except you have this check everywhere:
      Container.assertEventDispatchThread();

      which fails, as described above:
      http://bugs.sun.com/view_bug.do?bug_id=6424157
      due to EventQueue.isDispatchThread() failing on the "T1 vs T1*" distinction

      my request is this:
      do you think you could make
      Container.assertEventDispatchThread();
      less pedantic, and allow both "current and past/next" EDT threads to pass which are created during EventQueue push() / pop()

      thanks!

      Andrei

      1. patch-001-exit.patch
        1 kB
        Andrei Pozolotin
      2. patch_2011-05-29_assert-runner.patch
        1 kB
        Andrei Pozolotin
      3. patch_2011-05-27_edt-name.patch
        0.7 kB
        Andrei Pozolotin
      4. Container.java
        1 kB
        Andrei Pozolotin

        Issue Links

          Activity

          Hide
          Andrei Pozolotin added a comment -

          @Noel: great! works good now; thank you.

          Show
          Andrei Pozolotin added a comment - @Noel: great! works good now; thank you.
          Hide
          Noel Grandin added a comment -

          No, you can substitute your own main method and do whatever you need to do there, before calling DesktopApplicationContext#main()

          Show
          Noel Grandin added a comment - No, you can substitute your own main method and do whatever you need to do there, before calling DesktopApplicationContext#main()
          Hide
          Edvin Syse added a comment -

          Actually, an even cleaner approach would probably be to create an interface called EdtCheckerProvider or something, so that the Application class can implement this and return an EDT_CHECKER to be installed by the ApplicationContext even before creating the displayhost.

          Show
          Edvin Syse added a comment - Actually, an even cleaner approach would probably be to create an interface called EdtCheckerProvider or something, so that the Application class can implement this and return an EDT_CHECKER to be installed by the ApplicationContext even before creating the displayhost.
          Hide
          Edvin Syse added a comment -

          Nice, that should do it. But if you really need to change the default EDT_CHECKER, you won't be able to do so before the main() method of DesktopApplicationContext is run, and then the default EDIT_CHECKER has already been called multiple times. I think it would be much better to install the default EDT_CHECKER after Application#startup() is called from the ApplicationContext, provided no EDT_CHECKER was set by the #startup() method.

          Show
          Edvin Syse added a comment - Nice, that should do it. But if you really need to change the default EDT_CHECKER, you won't be able to do so before the main() method of DesktopApplicationContext is run, and then the default EDIT_CHECKER has already been called multiple times. I think it would be much better to install the default EDT_CHECKER after Application#startup() is called from the ApplicationContext, provided no EDT_CHECKER was set by the #startup() method.
          Hide
          Noel Grandin added a comment -

          Committed installable checker in rev 1129568

          Show
          Noel Grandin added a comment - Committed installable checker in rev 1129568
          Hide
          Andrei Pozolotin added a comment -

          @ Noel:
          please take a look on patch_2011-05-29_assert-runner.patch ;

          benefits:
          1) flexible / insertable / switchable;
          2) does not take much space in Container.java;
          3) much less perfomance hit when not active;
          4) allows not to blow up but reports violations instead;
          5) preserves original intent to inforce EDT contract;

          this is how I use it:

          ####################################

          public class App extends HostServiceProvider implements Application {

          private final Runnable assertRunner = new Runnable() {
          @Override
          public void run() {
          if (EventQueue.isDispatchThread())

          { return; }
          String name = Thread.currentThread().getName();
          if (name.equals("main")) { return; }

          if (name.equals("javawsApplicationMain"))

          { return; }
          if (name.startsWith("AWT-EventQueue-")) { return; }

          Exception e = new Exception("NON EDT THREAD : " + name);
          log.error("", e);
          }
          };

          @Override
          public synchronized void startup(Display display,
          org.apache.pivot.collections.Map<String, String> properties) {

          Container.setAssertRunner(assertRunner);

          @Override
          public synchronized boolean shutdown(final boolean isShutdownOptional) {

          Container.setAssertRunner(null);

          ####################################

          Show
          Andrei Pozolotin added a comment - @ Noel: please take a look on patch_2011-05-29_assert-runner.patch ; benefits: 1) flexible / insertable / switchable; 2) does not take much space in Container.java; 3) much less perfomance hit when not active; 4) allows not to blow up but reports violations instead; 5) preserves original intent to inforce EDT contract; this is how I use it: #################################### public class App extends HostServiceProvider implements Application { private final Runnable assertRunner = new Runnable() { @Override public void run() { if (EventQueue.isDispatchThread()) { return; } String name = Thread.currentThread().getName(); if (name.equals("main")) { return; } if (name.equals("javawsApplicationMain")) { return; } if (name.startsWith("AWT-EventQueue-")) { return; } Exception e = new Exception("NON EDT THREAD : " + name); log.error("", e); } }; @Override public synchronized void startup(Display display, org.apache.pivot.collections.Map<String, String> properties) { Container.setAssertRunner(assertRunner); @Override public synchronized boolean shutdown(final boolean isShutdownOptional) { Container.setAssertRunner(null); ####################################
          Show
          Andrei Pozolotin added a comment - @Edvin: you are right; the patch does not work for jnlp start; as described here: https://issues.apache.org/jira/browse/PIVOT-747?focusedCommentId=13040716&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13040716
          Hide
          Andrei Pozolotin added a comment -

          @Noel: thanks! testing now.

          Show
          Andrei Pozolotin added a comment - @Noel: thanks! testing now.
          Hide
          Edvin Syse added a comment -

          Sorry, I checked out from trunk, but it seems the patch isn't in trunk yet I guess all is fine, sorry for the confusion

          Show
          Edvin Syse added a comment - Sorry, I checked out from trunk, but it seems the patch isn't in trunk yet I guess all is fine, sorry for the confusion
          Hide
          Edvin Syse added a comment -

          I don't think this is enough for Java WebStart. The main thread there is called javawsApplicationMain. Could you add a check for this as well? Right now, Webstart is not working with Pivot as far as I understand.

          Show
          Edvin Syse added a comment - I don't think this is enough for Java WebStart. The main thread there is called javawsApplicationMain. Could you add a check for this as well? Right now, Webstart is not working with Pivot as far as I understand.
          Hide
          Noel Grandin added a comment -

          Fixed in rev 1129031

          Show
          Noel Grandin added a comment - Fixed in rev 1129031
          Hide
          Andrei Pozolotin added a comment -

          for your review:

          patch_2011-05-29_assert-runner.patch

          Show
          Andrei Pozolotin added a comment - for your review: patch_2011-05-29_assert-runner.patch
          Hide
          Edvin Syse added a comment -

          I can confirm the same problem. When I deploy with trunk and start as applet (via jnlp), everything is fine. If I start as WebStart, I get the IllegalStateException on startup, stacktrace is similar to Andrei's.

          Show
          Edvin Syse added a comment - I can confirm the same problem. When I deploy with trunk and start as applet (via jnlp), everything is fine. If I start as WebStart, I get the IllegalStateException on startup, stacktrace is similar to Andrei's.
          Hide
          Andrei Pozolotin added a comment -

          @Greg & @Noel:

          guys, in case you still thik this is "minor":

          I have a use case when under jnlp startup pivot blows up on the same assert,
          because there is a different "main" under sun plugin2 in browser, called "javawsApplicationMain"
          and I can not even rename the thread, because it happens too early;

          ##################################

          stack trace:

          java.lang.IllegalStateException: this method can only be called from the AWT event dispatch thread; javawsApplicationMain
          at org.apache.pivot.wtk.Container.assertEventDispatchThread(Container.java:853)
          at org.apache.pivot.wtk.WTKListenerList.add(WTKListenerList.java:36)
          at org.apache.pivot.wtk.skin.ComponentSkin.install(ComponentSkin.java:96)
          at org.apache.pivot.wtk.skin.ContainerSkin.install(ContainerSkin.java:129)
          at org.apache.pivot.wtk.skin.DisplaySkin.install(DisplaySkin.java:41)
          at org.apache.pivot.wtk.Component.setSkin(Component.java:749)
          at org.apache.pivot.wtk.Display.<init>(Display.java:31)
          at org.apache.pivot.wtk.ApplicationContext$DisplayHost.<init>(ApplicationContext.java:77)
          at org.apache.pivot.wtk.DesktopApplicationContext$DesktopDisplayHost.<init>(DesktopApplicationContext.java:90)
          at org.apache.pivot.wtk.DesktopApplicationContext.main(DesktopApplicationContext.java:537)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at com.sun.javaws.Launcher.executeApplication(Launcher.java:1804)
          at com.sun.javaws.Launcher.executeMainClass(Launcher.java:1750)
          at com.sun.javaws.Launcher.doLaunchApp(Launcher.java:1512)
          at com.sun.javaws.Launcher.run(Launcher.java:130)
          at java.lang.Thread.run(Thread.java:662)

          ##################################

          modified assert that prints thread name:

          public static final void assertEventDispatchThread() {
          /* Currently, application startup happens on the main thread, so we need to allow

          • that thread to modify WTK state.
            */
            if (!java.awt.EventQueue.isDispatchThread()
            && !Thread.currentThread().getName().equals("main")) { throw new IllegalStateException("this method can only be called from the AWT event dispatch thread; " + Thread.currentThread().getName()); }

            }

          ##################################

          and the test app - does nothing wrong, right?

          ##################################

          public class AppXXX implements Application {

          protected final Logger log = LoggerFactory.getLogger(getClass());

          public AppXXX()

          { log.info("init"); }

          //

          @Override
          public synchronized void startup(Display display,
          org.apache.pivot.collections.Map<String, String> properties)

          { log.debug("### pivot startup"); }

          @Override
          public synchronized boolean shutdown(final boolean isShutdownOptional) {

          log.debug("### pivot shutdown; isOptional : {};", isShutdownOptional);

          return true;

          }

          //

          @Override
          public void suspend() throws Exception

          { // not used }

          @Override
          public void resume() throws Exception { // not used }

          }

          ##################################

          Show
          Andrei Pozolotin added a comment - @Greg & @Noel: guys, in case you still thik this is "minor": I have a use case when under jnlp startup pivot blows up on the same assert, because there is a different "main" under sun plugin2 in browser, called "javawsApplicationMain" and I can not even rename the thread, because it happens too early; ################################## stack trace: java.lang.IllegalStateException: this method can only be called from the AWT event dispatch thread; javawsApplicationMain at org.apache.pivot.wtk.Container.assertEventDispatchThread(Container.java:853) at org.apache.pivot.wtk.WTKListenerList.add(WTKListenerList.java:36) at org.apache.pivot.wtk.skin.ComponentSkin.install(ComponentSkin.java:96) at org.apache.pivot.wtk.skin.ContainerSkin.install(ContainerSkin.java:129) at org.apache.pivot.wtk.skin.DisplaySkin.install(DisplaySkin.java:41) at org.apache.pivot.wtk.Component.setSkin(Component.java:749) at org.apache.pivot.wtk.Display.<init>(Display.java:31) at org.apache.pivot.wtk.ApplicationContext$DisplayHost.<init>(ApplicationContext.java:77) at org.apache.pivot.wtk.DesktopApplicationContext$DesktopDisplayHost.<init>(DesktopApplicationContext.java:90) at org.apache.pivot.wtk.DesktopApplicationContext.main(DesktopApplicationContext.java:537) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at com.sun.javaws.Launcher.executeApplication(Launcher.java:1804) at com.sun.javaws.Launcher.executeMainClass(Launcher.java:1750) at com.sun.javaws.Launcher.doLaunchApp(Launcher.java:1512) at com.sun.javaws.Launcher.run(Launcher.java:130) at java.lang.Thread.run(Thread.java:662) ################################## modified assert that prints thread name: public static final void assertEventDispatchThread() { /* Currently, application startup happens on the main thread, so we need to allow that thread to modify WTK state. */ if (!java.awt.EventQueue.isDispatchThread() && !Thread.currentThread().getName().equals("main")) { throw new IllegalStateException("this method can only be called from the AWT event dispatch thread; " + Thread.currentThread().getName()); } } ################################## and the test app - does nothing wrong, right? ################################## public class AppXXX implements Application { protected final Logger log = LoggerFactory.getLogger(getClass()); public AppXXX() { log.info("init"); } // @Override public synchronized void startup(Display display, org.apache.pivot.collections.Map<String, String> properties) { log.debug("### pivot startup"); } @Override public synchronized boolean shutdown(final boolean isShutdownOptional) { log.debug("### pivot shutdown; isOptional : {};", isShutdownOptional); return true; } // @Override public void suspend() throws Exception { // not used } @Override public void resume() throws Exception { // not used } } ##################################
          Hide
          Andrei Pozolotin added a comment -

          @Greg, @Noel,
          guys, while you still thinking; can you please commit
          patch_2011-05-27_edt-name.patch
          ?

          Show
          Andrei Pozolotin added a comment - @Greg, @Noel, guys, while you still thinking; can you please commit patch_2011-05-27_edt-name.patch ?
          Hide
          Andrei Pozolotin added a comment -

          or, do you think you can agree on this small change to the current assertEventDispatchThread() [ see: startsWith("AWT-EventQueue-") ]:
          ?
          ####################################
          public static final void assertEventDispatchThread() {
          /*

          • Currently, application startup happens on the main thread, so we need
          • to allow that thread to modify WTK state.
            */
            if (!java.awt.EventQueue.isDispatchThread()
            && !Thread.currentThread().getName().equals("main")
            && !Thread.currentThread().getName()
            .startsWith("AWT-EventQueue-")) { throw new IllegalStateException( "this method can only be called from the AWT event dispatch thread"); }

            }
            ####################################

          Show
          Andrei Pozolotin added a comment - or, do you think you can agree on this small change to the current assertEventDispatchThread() [ see: startsWith("AWT-EventQueue-") ]: ? #################################### public static final void assertEventDispatchThread() { /* Currently, application startup happens on the main thread, so we need to allow that thread to modify WTK state. */ if (!java.awt.EventQueue.isDispatchThread() && !Thread.currentThread().getName().equals("main") && !Thread.currentThread().getName() .startsWith("AWT-EventQueue-")) { throw new IllegalStateException( "this method can only be called from the AWT event dispatch thread"); } } ####################################
          Hide
          Andrei Pozolotin added a comment -

          great! decided, then. should I submit a patch?

          Show
          Andrei Pozolotin added a comment - great! decided, then. should I submit a patch?
          Hide
          Noel Grandin added a comment -

          I'm in agreement with Greg, trying to fix Andrei's use-case is getting too complex.

          So yes, I think we should make it possible to turn it off with a system property.

          I think it's actually fairly robust in general - very few developers play the kind of EDT-switching tricks that Andrei does.

          For those developers not using such tricks, it provides early-on checking for a class of problems that is otherwise hard to debug.

          I also note from reading the bugs referenced by Andrei, that this particular problem is fixed in JDK7, so when that comes around, Andrei will not longer need to turn this off.

          Show
          Noel Grandin added a comment - I'm in agreement with Greg, trying to fix Andrei's use-case is getting too complex. So yes, I think we should make it possible to turn it off with a system property. I think it's actually fairly robust in general - very few developers play the kind of EDT-switching tricks that Andrei does. For those developers not using such tricks, it provides early-on checking for a class of problems that is otherwise hard to debug. I also note from reading the bugs referenced by Andrei, that this particular problem is fixed in JDK7, so when that comes around, Andrei will not longer need to turn this off.
          Hide
          Andrei Pozolotin added a comment -

          re: "should remain" - yes, I vote it should remain, but be more flexible

          Show
          Andrei Pozolotin added a comment - re: "should remain" - yes, I vote it should remain, but be more flexible
          Hide
          Andrei Pozolotin added a comment -

          well, I am at loss, but you are the boss!
          I still would prefer police injection as above
          https://issues.apache.org/jira/browse/PIVOT-747?focusedCommentId=13039125&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13039125

          and yes, I understand you do not want to pollute nice api with ugly things like this.
          but you can take this check out of the Container.java and place in a separate, say Policy.java class,
          which can continue to collect cases like this

          Show
          Andrei Pozolotin added a comment - well, I am at loss, but you are the boss! I still would prefer police injection as above https://issues.apache.org/jira/browse/PIVOT-747?focusedCommentId=13039125&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13039125 and yes, I understand you do not want to pollute nice api with ugly things like this. but you can take this check out of the Container.java and place in a separate, say Policy.java class, which can continue to collect cases like this
          Hide
          Greg Brown added a comment -

          My preference would actually be to remove the check altogether. But if others feel strongly that it should remain, I think a flag would be preferable.

          Show
          Greg Brown added a comment - My preference would actually be to remove the check altogether. But if others feel strongly that it should remain, I think a flag would be preferable.
          Hide
          Greg Brown added a comment -

          > re: "such a narrow use case" - sorry, but looks very wide in my eyes

          Understood. But from the perspective of a Pivot platform developer, this is a narrow use case. And we generally try to avoid overburdening the platform with code to address narrow use cases.

          Show
          Greg Brown added a comment - > re: "such a narrow use case" - sorry, but looks very wide in my eyes Understood. But from the perspective of a Pivot platform developer, this is a narrow use case. And we generally try to avoid overburdening the platform with code to address narrow use cases.
          Hide
          Andrei Pozolotin added a comment -

          re: "flag would be simple" - yes;
          but it would also limit you to single way of checking; and will force another negotiation when someone else needs some other check enforced by pivot

          Show
          Andrei Pozolotin added a comment - re: "flag would be simple" - yes; but it would also limit you to single way of checking; and will force another negotiation when someone else needs some other check enforced by pivot
          Hide
          Andrei Pozolotin added a comment -

          also: thead name "main" based check: some frameworks (and people!) rename threads early, before pivot sees it.

          Show
          Andrei Pozolotin added a comment - also: thead name "main" based check: some frameworks (and people!) rename threads early, before pivot sees it.
          Hide
          Andrei Pozolotin added a comment -

          re: "such a narrow use case" - sorry, but looks very wide in my eyes

          but probably more problematic is the philosophy of enforcing a policy which is buggy in itself;

          for example, if I am creating an osgi-based platform, where people will contribute their own plugins,
          I'd rather have a way to collect "edt policy violations" form their plugins and report back to them in a meaninful way,
          instead of to blow up entire application at run time.

          Show
          Andrei Pozolotin added a comment - re: "such a narrow use case" - sorry, but looks very wide in my eyes but probably more problematic is the philosophy of enforcing a policy which is buggy in itself; for example, if I am creating an osgi-based platform, where people will contribute their own plugins, I'd rather have a way to collect "edt policy violations" form their plugins and report back to them in a meaninful way, instead of to blow up entire application at run time.
          Hide
          Greg Brown added a comment -

          I think a flag would be simpler. It could be set via a system property.

          Show
          Greg Brown added a comment - I think a flag would be simpler. It could be set via a system property.
          Hide
          Andrei Pozolotin added a comment -

          how about this:

          ################################

          public class Container1 {

          private volatile static Runnable policeEDT;

          public static Runnable getPoliceEDT()

          { return policeEDT; }

          public static void setPoliceEDT(Runnable police)

          { policeEDT = police; }

          //

          static {
          Runnable defaultPoliceEDT = new Runnable() {
          @Override
          public void run()

          { // use current pivot checker? }

          };
          setPoliceEDT(defaultPoliceEDT);
          }

          public static final void assertEventDispatchThread()

          { getPoliceEDT().run(); }

          }

          ################################

          Show
          Andrei Pozolotin added a comment - how about this: ################################ public class Container1 { private volatile static Runnable policeEDT; public static Runnable getPoliceEDT() { return policeEDT; } public static void setPoliceEDT(Runnable police) { policeEDT = police; } // static { Runnable defaultPoliceEDT = new Runnable() { @Override public void run() { // use current pivot checker? } }; setPoliceEDT(defaultPoliceEDT); } public static final void assertEventDispatchThread() { getPoliceEDT().run(); } } ################################
          Hide
          Greg Brown added a comment -

          Not sure if it would be best to enable or disable by default. Noel, what do you think?

          Show
          Greg Brown added a comment - Not sure if it would be best to enable or disable by default. Noel, what do you think?
          Hide
          Sandro Martini added a comment -

          Hi all,
          Greg I agree with you, this is an useful feature, but probably it would be better to have a (simple) way to enable/disable, for example using a system property.
          And by default do you think it's better to have enabled or disabled ?

          For a mid/long term solution (for this and maybe other things) could we think to add environment-dependent behavior (but maybe this could be part of the 2.1 release) (DEVELOPMENT, TEST, PREPRODUCTION, PRODUCTION, etc ...) like in Grails, Griffon, Spring 3.1.x, etc ? But all this could be too much ...
          Some reference here:
          http://www.grails.org/Environments
          http://griffon-user.3225736.n2.nabble.com/Problems-with-Environment-current-td5138933.html
          If you think this could be interesting tell me and I'll add a ticket for the 2.1 on this.

          Bye

          Show
          Sandro Martini added a comment - Hi all, Greg I agree with you, this is an useful feature, but probably it would be better to have a (simple) way to enable/disable, for example using a system property. And by default do you think it's better to have enabled or disabled ? For a mid/long term solution (for this and maybe other things) could we think to add environment-dependent behavior (but maybe this could be part of the 2.1 release) (DEVELOPMENT, TEST, PREPRODUCTION, PRODUCTION, etc ...) like in Grails, Griffon, Spring 3.1.x, etc ? But all this could be too much ... Some reference here: http://www.grails.org/Environments http://griffon-user.3225736.n2.nabble.com/Problems-with-Environment-current-td5138933.html If you think this could be interesting tell me and I'll add a ticket for the 2.1 on this. Bye
          Hide
          Greg Brown added a comment -

          My personal feeling is that this is getting a bit too complex, especially for such a narrow use case. Rather than synchronizing maybe it would be better to simply provide a means to turn the EDT check off.

          Show
          Greg Brown added a comment - My personal feeling is that this is getting a bit too complex, especially for such a narrow use case. Rather than synchronizing maybe it would be better to simply provide a means to turn the EDT check off.
          Hide
          Andrei Pozolotin added a comment -

          it is no more hack than checking for "main" in thread name, would you agree?

          Show
          Andrei Pozolotin added a comment - it is no more hack than checking for "main" in thread name, would you agree?
          Hide
          Andrei Pozolotin added a comment -

          re: "circumstances" - the original issue of pivot blowing up with exceptions under push()/pop();

          my "name hack idea" is just replacement for your isEventDispatchThread():

          ##############################################

          public static final void assertEventDispatchThread()

          { assert isEventDispatchThread(); }

          public static final synchronized boolean isEventDispatchThread()

          { String threadName = Thread.currentThread().getName(); return EventQueue.isDispatchThread() || threadName.equals("main") || threadName.startsWith("AWT-EventQueue-"); }

          ##############################################

          Show
          Andrei Pozolotin added a comment - re: "circumstances" - the original issue of pivot blowing up with exceptions under push()/pop(); my "name hack idea" is just replacement for your isEventDispatchThread(): ############################################## public static final void assertEventDispatchThread() { assert isEventDispatchThread(); } public static final synchronized boolean isEventDispatchThread() { String threadName = Thread.currentThread().getName(); return EventQueue.isDispatchThread() || threadName.equals("main") || threadName.startsWith("AWT-EventQueue-"); } ##############################################
          Hide
          Greg Brown added a comment -

          Under what circumstances might an application want to call this method?

          Show
          Greg Brown added a comment - Under what circumstances might an application want to call this method?
          Hide
          Andrei Pozolotin added a comment -

          still another little hack idea: why not permit more names!

          public static boolean isProperEDT()

          { /* * Currently, application startup happens on the main thread, so we need * to allow that thread to modify WTK state. */ String threadName = Thread.currentThread().getName(); return EventQueue.isDispatchThread() || threadName.equals("main") || threadName.startsWith("AWT-EventQueue-"); }
          Show
          Andrei Pozolotin added a comment - still another little hack idea: why not permit more names! public static boolean isProperEDT() { /* * Currently, application startup happens on the main thread, so we need * to allow that thread to modify WTK state. */ String threadName = Thread.currentThread().getName(); return EventQueue.isDispatchThread() || threadName.equals("main") || threadName.startsWith("AWT-EventQueue-"); }
          Hide
          Andrei Pozolotin added a comment -

          @Greg & @Noel:
          attached Container.java shows an idea of how to make edt police more people friendly; what do you think?

          Show
          Andrei Pozolotin added a comment - @Greg & @Noel: attached Container.java shows an idea of how to make edt police more people friendly; what do you think?
          Hide
          Andrei Pozolotin added a comment -

          @Greg:
          a) re: "applied a modified version of the patch" - thank you;
          b) re: "encourage you to test" - sorry; I will;

          Show
          Andrei Pozolotin added a comment - @Greg: a) re: "applied a modified version of the patch" - thank you; b) re: "encourage you to test" - sorry; I will;
          Hide
          Greg Brown added a comment -

          @Noel - I don't think we should make assertEventDispatchThread() synchronized. That will incur a performance hit which, in my opinion, isn't justified for what is essentially debug code. It may be possible to avoid that penalty by wrapping the actual check in an assert:

          public static final void assertEventDispatchThread()

          { assert isEventDispatchThread(); }

          public static final synchronized boolean isEventDispatchThread()

          { return (java.awt.EventQueue.isDispatchThread() && !Thread.currentThread().getName().equals("main")); }

          However, it would probably be best to avoid this if possible.

          Show
          Greg Brown added a comment - @Noel - I don't think we should make assertEventDispatchThread() synchronized. That will incur a performance hit which, in my opinion, isn't justified for what is essentially debug code. It may be possible to avoid that penalty by wrapping the actual check in an assert: public static final void assertEventDispatchThread() { assert isEventDispatchThread(); } public static final synchronized boolean isEventDispatchThread() { return (java.awt.EventQueue.isDispatchThread() && !Thread.currentThread().getName().equals("main")); } However, it would probably be best to avoid this if possible.
          Hide
          Greg Brown added a comment -

          I have applied a modified version of the patch as the original didn't compile. I'd encourage you to test any future patches more thoroughly before submitting them. I also changed the name of the argument from "isOptional" to "optional" since Pivot's coding conventions don't include the "is" prefix in variable names, just getter methods.

          Show
          Greg Brown added a comment - I have applied a modified version of the patch as the original didn't compile. I'd encourage you to test any future patches more thoroughly before submitting them. I also changed the name of the argument from "isOptional" to "optional" since Pivot's coding conventions don't include the "is" prefix in variable names, just getter methods.
          Hide
          Noel Grandin added a comment -

          Thanks for your analysis Andrei.

          There is not much I can do to fix EventQueue#isDispatchThread.

          But does it help your situation if you change the Container#assertEventDispatchThread method to be synchronized?

          That will force the VM to emit a memory barrier, which will at least enforce that it can see recent memory writes.

          Show
          Noel Grandin added a comment - Thanks for your analysis Andrei. There is not much I can do to fix EventQueue#isDispatchThread. But does it help your situation if you change the Container#assertEventDispatchThread method to be synchronized? That will force the VM to emit a memory barrier, which will at least enforce that it can see recent memory writes.
          Hide
          Andrei Pozolotin added a comment -

          Noel, hello again!

          1) I think I found the reason why your isDispatchThread() blows up on push() / pop():

          2) both push and pop are synchronized :

          public synchronized void push(EventQueue newEventQueue) {
          synchronized (newEventQueue) {

          protected void pop() throws EmptyStackException {
          synchronized ((prev != null) ? prev : this) {
          synchronized(this) {

          3) but isDispatchThread() is not synchronized, hence it can see all kind of transitory phases;

          public static boolean isDispatchThread() {
          EventQueue eq = Toolkit.getEventQueue();
          EventQueue next = eq.nextQueue;
          while (next != null)

          { eq = next; next = eq.nextQueue; }

          return (Thread.currentThread() == eq.dispatchThread);
          }

          for example I have a test case which, with proper timing,
          blows up both source and target EDT threads during push/pop switch!

          4) so it seems, if you insist on EventQueue Pivot Police ®
          we would need a better isDispatchThread() implementatoion;

          5) bottom line: isDispatchThread() is a good guard for EDT vs non-EDT,
          but does not work for EDT vs EDT;

          Cheers,

          Andrei.

          Show
          Andrei Pozolotin added a comment - Noel, hello again! 1) I think I found the reason why your isDispatchThread() blows up on push() / pop(): 2) both push and pop are synchronized : public synchronized void push(EventQueue newEventQueue) { synchronized (newEventQueue) { protected void pop() throws EmptyStackException { synchronized ((prev != null) ? prev : this) { synchronized(this) { 3) but isDispatchThread() is not synchronized, hence it can see all kind of transitory phases; public static boolean isDispatchThread() { EventQueue eq = Toolkit.getEventQueue(); EventQueue next = eq.nextQueue; while (next != null) { eq = next; next = eq.nextQueue; } return (Thread.currentThread() == eq.dispatchThread); } for example I have a test case which, with proper timing, blows up both source and target EDT threads during push/pop switch! 4) so it seems, if you insist on EventQueue Pivot Police ® we would need a better isDispatchThread() implementatoion; 5) bottom line: isDispatchThread() is a good guard for EDT vs non-EDT, but does not work for EDT vs EDT; Cheers, Andrei.
          Hide
          Andrei Pozolotin added a comment -

          Greg: if pach is ok, can you please apply?

          Noel: can you please relax your assertEventDispatchThread:
          just log error but do not blow up with exception? or make it optional to blow up or not?
          I still have use cases where AWT works fine w/o being "politically correct".

          thanks!

          Andrei.

          Show
          Andrei Pozolotin added a comment - Greg: if pach is ok, can you please apply? Noel: can you please relax your assertEventDispatchThread: just log error but do not blow up with exception? or make it optional to blow up or not? I still have use cases where AWT works fine w/o being "politically correct". thanks! Andrei.
          Hide
          Andrei Pozolotin added a comment -

          great! yes.

          Show
          Andrei Pozolotin added a comment - great! yes.
          Hide
          Greg Brown added a comment -

          That seems reasonable. Can you submit a patch?

          Show
          Greg Brown added a comment - That seems reasonable. Can you submit a patch?
          Hide
          Andrei Pozolotin added a comment -

          do you think you could expand

          private static class DesktopDisplayHost extends DisplayHost {

          public static boolean exit() {

          with extra:

          public static boolean exit(boolean isOptional) {

          and propogate isOptional to shutdown(boolean isOptional) ?

          as flag signaling gets convoluted, and I've got no static fields

          Show
          Andrei Pozolotin added a comment - do you think you could expand private static class DesktopDisplayHost extends DisplayHost { public static boolean exit() { with extra: public static boolean exit(boolean isOptional) { and propogate isOptional to shutdown(boolean isOptional) ? as flag signaling gets convoluted, and I've got no static fields
          Hide
          Andrei Pozolotin added a comment -

          great idea, thank you! I will try to see if it works for me and report back.

          Show
          Andrei Pozolotin added a comment - great idea, thank you! I will try to see if it works for me and report back.
          Hide
          Greg Brown added a comment -

          Can't you just return true from shutdown() to prevent the Pivot app from closing? Then when your OSGi shutdown hook is called, you set a flag in your app and call DesktopApplicationContext.exit(), similar to what is implemented here:

          http://svn.apache.org/repos/asf/pivot/trunk/tests/src/org/apache/pivot/tests/ShutdownTest.java

          Show
          Greg Brown added a comment - Can't you just return true from shutdown() to prevent the Pivot app from closing? Then when your OSGi shutdown hook is called, you set a flag in your app and call DesktopApplicationContext.exit(), similar to what is implemented here: http://svn.apache.org/repos/asf/pivot/trunk/tests/src/org/apache/pivot/tests/ShutdownTest.java
          Hide
          Andrei Pozolotin added a comment -

          re: "which correctly traverses the push()/pop() hierarchy, so there shouldn't be a problem there. "

          well, it does not;

          the failure scenario is this:

          a) I issue pop() in the "old EDT", while some pivot components
          are still being closed in the "new EDT";

          b) this closing results in some pivot event listener trying to unregster some pivot component,
          which calls your assert, and it fails, since "new EDT" is no longer "valid" according to EventQueue.isDispatchThread();

          Show
          Andrei Pozolotin added a comment - re: "which correctly traverses the push()/pop() hierarchy, so there shouldn't be a problem there. " well, it does not; the failure scenario is this: a) I issue pop() in the "old EDT", while some pivot components are still being closed in the "new EDT"; b) this closing results in some pivot event listener trying to unregster some pivot component, which calls your assert, and it fails, since "new EDT" is no longer "valid" according to EventQueue.isDispatchThread();
          Hide
          Andrei Pozolotin added a comment -

          Noel: thank you very much for looking into this.

          my problem is this:

          1) I receive shutdown(optional==true) from the user;

          2) before I run (or cancel) actual shutdown, including osgiShutdown()
          I need to perfom negotiation with the user;

          3) this negotiation happens in osgi bundle wich runs on non-ETD thread
          and which uses EventQueue.invokeLater() to converse with the user;

          4) while the negotiation is going on, I must block original ETD which called shutdown()
          (or else pivot will just shut me down) and permit new working EDT in the meanwhile;

          5) this is pretty much the same problem as net beans people discussed some time back:
          http://bugs.sun.com/view_bug.do?bug_id=6424157
          http://netbeans.org/bugzilla/show_bug.cgi?id=90590

          am I making more sense?

          Andrei.

          Show
          Andrei Pozolotin added a comment - Noel: thank you very much for looking into this. my problem is this: 1) I receive shutdown(optional==true) from the user; 2) before I run (or cancel) actual shutdown, including osgiShutdown() I need to perfom negotiation with the user; 3) this negotiation happens in osgi bundle wich runs on non-ETD thread and which uses EventQueue.invokeLater() to converse with the user; 4) while the negotiation is going on, I must block original ETD which called shutdown() (or else pivot will just shut me down) and permit new working EDT in the meanwhile; 5) this is pretty much the same problem as net beans people discussed some time back: http://bugs.sun.com/view_bug.do?bug_id=6424157 http://netbeans.org/bugzilla/show_bug.cgi?id=90590 am I making more sense? Andrei.
          Hide
          Noel Grandin added a comment -

          Firstly, Container.assertEventDispatchThread() calls into EventQueue.isDispatchThread(), which correctly traverses the push()/pop() hierarchy, so there shouldn't be a problem there.

          Secondly, I don't understand why you need such a complicated solution. Why can you not simply call osgiShutdown() directly from your
          Application#shutdown(boolean) ?
          And why not simply call osgiStartup() from Application#startup ?

          Show
          Noel Grandin added a comment - Firstly, Container.assertEventDispatchThread() calls into EventQueue.isDispatchThread(), which correctly traverses the push()/pop() hierarchy, so there shouldn't be a problem there. Secondly, I don't understand why you need such a complicated solution. Why can you not simply call osgiShutdown() directly from your Application#shutdown(boolean) ? And why not simply call osgiStartup() from Application#startup ?
          Hide
          Greg Brown added a comment -

          The application lifecycle methods are defined to be called from the EDT, so changing that would introduce other problems. Maybe Noel can comment on the other issues - he added the assertEventDispatchThread() functionality.

          Show
          Greg Brown added a comment - The application lifecycle methods are defined to be called from the EDT, so changing that would introduce other problems. Maybe Noel can comment on the other issues - he added the assertEventDispatchThread() functionality.
          Hide
          Andrei Pozolotin added a comment -

          or may be you can call startup() & shutdown() from non-EDT pool thread?

          Show
          Andrei Pozolotin added a comment - or may be you can call startup() & shutdown() from non-EDT pool thread?
          Hide
          Andrei Pozolotin added a comment -

          and use cases:

          ##################

          @Override
          public void startup(Display display,
          org.apache.pivot.collections.Map<String, String> properties)
          throws Exception {

          new EQ_Runner("# OSGI START") {
          @Override
          protected void runCore() throws Exception

          { osgiStartup(); }

          }.execute();

          ##################

          @Override
          public boolean shutdown(final boolean isOptional) throws Exception {

          final Value<Boolean> isCanceled = new Value<Boolean>(false);

          new EQ_Runner("# OSGI FINISH") {
          @Override
          protected void runCore() throws Exception {
          isCanceled.set(isOptional && isShutdownCanceled());
          if (isCanceled.get())

          { return; }

          osgiShutdown();
          }
          }.execute();

          ##################

          Show
          Andrei Pozolotin added a comment - and use cases: ################## @Override public void startup(Display display, org.apache.pivot.collections.Map<String, String> properties) throws Exception { new EQ_Runner("# OSGI START") { @Override protected void runCore() throws Exception { osgiStartup(); } }.execute(); ################## @Override public boolean shutdown(final boolean isOptional) throws Exception { final Value<Boolean> isCanceled = new Value<Boolean>(false); new EQ_Runner("# OSGI FINISH") { @Override protected void runCore() throws Exception { isCanceled.set(isOptional && isShutdownCanceled()); if (isCanceled.get()) { return; } osgiShutdown(); } }.execute(); ##################
          Hide
          Andrei Pozolotin added a comment -

          this is my current pusher-popper

          public abstract class EQ_Runner implements Runnable {

          protected final Logger log = LoggerFactory.getLogger(getClass());

          private final String name;

          public EQ_Runner(String name)

          { this.name = name; }

          private boolean isDispatchThread()

          { return EventQueue.isDispatchThread(); }

          private void assertEventDispatchThread() {
          if (isDispatchThread())

          { return; }

          else

          { String message = "must be in EDT"; log.error(message); throw new IllegalStateException(message); }

          }

          private EventQueue getSystemQueue()

          { return Toolkit.getDefaultToolkit().getSystemEventQueue(); }

          private class ExtraQueue extends EventQueue {
          @Override
          public void pop() {
          try

          { super.pop(); }

          catch (Throwable e)

          { log.error("", e); }

          }
          }

          protected abstract void runCore() throws Exception;

          @Override
          public final void run() {
          try

          { runCore(); }

          catch (Throwable e)

          { log.error("", e); }

          }

          // http://bugs.sun.com/view_bug.do?bug_id=6424157
          // http://netbeans.org/bugzilla/show_bug.cgi?id=90590
          // http://code.google.com/p/javafxdemos/source/browse/ModalDialog/src/modaldialog/EventQueueUtils.java

          public final void execute() {

          // main EQ; main EDT;
          assertEventDispatchThread();

          // log.debug("START : {} : {}", isDispatchThread(), getSystemQueue());
          // log.debug("### thread id : {}", Thread.currentThread().getId());

          EventQueue mainQueue = getSystemQueue();
          ExtraQueue workQueue = new ExtraQueue();

          Thread worker = new Thread(this, name);

          // context: switch new
          // drain main EQ into work EQ
          // start work EDT
          mainQueue.push(workQueue);

          // worker uses work EDT
          worker.start();

          // block old main EDT
          try

          { worker.join(); }

          catch (InterruptedException e)

          { e.printStackTrace(); }

          // context: restore old
          // drain work EQ into main EQ;
          // stop old main EDT; start new main EDT; stop work EDT;
          workQueue.pop();

          // NOTE: this is no longer a EDT form "pedantic point of view"
          // assertEventDispatchThread();
          // log.debug("FINISH : {} : {}", isDispatchThread(), getSystemQueue());

          }

          }

          Show
          Andrei Pozolotin added a comment - this is my current pusher-popper public abstract class EQ_Runner implements Runnable { protected final Logger log = LoggerFactory.getLogger(getClass()); private final String name; public EQ_Runner(String name) { this.name = name; } private boolean isDispatchThread() { return EventQueue.isDispatchThread(); } private void assertEventDispatchThread() { if (isDispatchThread()) { return; } else { String message = "must be in EDT"; log.error(message); throw new IllegalStateException(message); } } private EventQueue getSystemQueue() { return Toolkit.getDefaultToolkit().getSystemEventQueue(); } private class ExtraQueue extends EventQueue { @Override public void pop() { try { super.pop(); } catch (Throwable e) { log.error("", e); } } } protected abstract void runCore() throws Exception; @Override public final void run() { try { runCore(); } catch (Throwable e) { log.error("", e); } } // http://bugs.sun.com/view_bug.do?bug_id=6424157 // http://netbeans.org/bugzilla/show_bug.cgi?id=90590 // http://code.google.com/p/javafxdemos/source/browse/ModalDialog/src/modaldialog/EventQueueUtils.java public final void execute() { // main EQ; main EDT; assertEventDispatchThread(); // log.debug("START : {} : {}", isDispatchThread(), getSystemQueue()); // log.debug("### thread id : {}", Thread.currentThread().getId()); EventQueue mainQueue = getSystemQueue(); ExtraQueue workQueue = new ExtraQueue(); Thread worker = new Thread(this, name); // context: switch new // drain main EQ into work EQ // start work EDT mainQueue.push(workQueue); // worker uses work EDT worker.start(); // block old main EDT try { worker.join(); } catch (InterruptedException e) { e.printStackTrace(); } // context: restore old // drain work EQ into main EQ; // stop old main EDT; start new main EDT; stop work EDT; workQueue.pop(); // NOTE: this is no longer a EDT form "pedantic point of view" // assertEventDispatchThread(); // log.debug("FINISH : {} : {}", isDispatchThread(), getSystemQueue()); } }

            People

            • Assignee:
              Noel Grandin
              Reporter:
              Andrei Pozolotin
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development