Re: [cp-patches] FYI: javax.swing.Timer spurious Thread.sleep() interrupts fixlet

[Mark Wielaard]

Hi,

On Wed, 2005-11-02 at 22:30 +0100, Mark Wielaard wrote:
> While debugging some other swing issue we found an small issue with
> Timer.Waker ending prematurely if sleep() would be spuriously
> interrupted. This makes sure the Waker only terminates when really
> requested by Timer.stop().
> 
> 2005-11-02  Mark Wielaard  <address@hidden>
> 
>     * javax/swing/Timer.java (Waker.run): Only break out when !running.

That patch was a bit too simple. Lillian noticed some problems with her
test code. This patch uses queueLock to properly synchronize starting
and stopping the Waker.

2005-11-02  Mark Wielaard  <address@hidden>

    * javax/swing/Timer.java (Waker.run): Synchronize on queueLock and use
    queueLock.wait(), not Thread.sleep().
    (start): Synchronize on queueLock and check whether waker is null.
    (stop): Synchronize on queueLock and notifyAll().

Committed,

Mark

Index: javax/swing/Timer.java
===================================================================
RCS file: /cvsroot/classpath/classpath/javax/swing/Timer.java,v
retrieving revision 1.22
diff -u -r1.22 Timer.java
--- javax/swing/Timer.java      2 Nov 2005 21:30:41 -0000       1.22
+++ javax/swing/Timer.java      2 Nov 2005 23:59:48 -0000
@@ -65,43 +65,50 @@
      */
     public void run()
     {
-      running = true;
       try
         {
-          try
-            {
-              sleep(initialDelay);
-            }
-          catch (InterruptedException e)
+          synchronized (queueLock)
             {
+              try
+                {
+                  queueLock.wait(initialDelay);
+                }
+              catch (InterruptedException e)
+                {
+                  // Ignored
+                }
+
               if (!running)
                 return;
-            }
 
-          queueEvent();
+              queueEvent();
 
-          if (repeats)
-            while (running)
-              {
-                try
-                  {
-                    sleep(delay);
-                  }
-                catch (InterruptedException e)
+              if (repeats)
+                while (running)
                   {
+                    try
+                      {
+                        queueLock.wait(delay);
+                      }
+                    catch (InterruptedException e)
+                      {
+                         // Ignored
+                      }
+
                     if (!running)
                       break;
-                  }
-                queueEvent();
 
-                if (logTimers)
-                  System.out.println("javax.swing.Timer -> clocktick");
+                    queueEvent();
 
-                if ( ! repeats)
-                  break;
-              }
-          running = false;
-        }
+                    if (logTimers)
+                      System.out.println("javax.swing.Timer -> clocktick");
+
+                    if (!repeats)
+                      break;
+                  }
+              running = false;
+            }
+       }
       finally
         {
           // The timer is no longer running.
@@ -408,10 +415,13 @@
    */
   public void start()
   {
-    if (isRunning())
-      return;
-    waker = new Waker();
-    waker.start();
+    synchronized (queueLock)
+      {
+       if (waker != null)
+         return;
+       waker = new Waker();
+       waker.start();
+      }
   }
 
   /**
@@ -419,12 +429,13 @@
    */
   public void stop()
   {
-    running = false;
-    if (waker != null)
-      waker.interrupt();
     synchronized (queueLock)
       {
+       running = false;
         queue = 0;
+       if (waker != null)
+         queueLock.notifyAll();
+       waker = null;
       }
   }

signature.asc
Description: This is a digitally signed message part