<AWT Dev> wrong logic to prevent recursion in SunToolkit.flushPendingEvents()

Artem Ananiev artem.ananiev at oracle.com
Mon Jul 2 11:58:33 PDT 2012


Hi, Peter,

thanks for reporting this issue, it's clearly a bug in SunToolkit. I 
think it will be fixed as a part of 7159230.

See more comments below.

On 6/28/2012 11:53 AM, Peter Levart wrote:
> Also, this is another spot that uses it's own synchronizer to serialize access
> and calls into the PostEventQueue which is also synchronized (currently by
> using synchronized methods but as I proposed in a previous message "deadlock
> involving ..." could be using a global mutex) so it is prone to deadlock
> "bugs".
>
> I would rather use a ThreadLocal flag here to prevent re-entrancy from the same
> thread. Multiple threads can enter flushPendingEvents() with no danger since
> PostEventQueue.flush() is already synchronized.

flushPendingEvents() is not called from PostEventQueue.flush(). Instead, 
it's mostly called from EventQueue to make sure all the already posted 
events are processed. That's why we need synchronization in 
flushPendingEvents().

> For example like this:
>
>      private static final ThreadLocal<Boolean>  isFlushingPendingEvents =
>        new ThreadLocal<Boolean>();
>
>      /*
>       * Flush any pending events which haven't been posted to the AWT
>       * EventQueue yet.
>       */
>      public static void flushPendingEvents()  {
>          boolean wasFlushing = isFlushingPendingEvents.get() != null;
>          try {

Here the thread may be interrupted, and "wasFlushing" may become stale.

>              // Don't call flushPendingEvents() recursively
>              if (!wasFlushing) {
>                  isFlushingPendingEvents.set(true);
>                  AppContext appContext = AppContext.getAppContext();
>                  PostEventQueue postEventQueue =
>                      (PostEventQueue)appContext.get(POST_EVENT_QUEUE_KEY);
>                  if (postEventQueue != null) {
>                      postEventQueue.flush();
>                  }
>              }
>          } finally {
>             if (!wasFlushing) isFlushingPendingEvents.remove();
>          }
>      }

The easiest fix can be to check "isFlushingPendingEvents" in the 
beginning of the method:

         flushLock.lock();
         boolean wasFlushing = isFlushingPendingEvents;
         try {
             // Don't call flushPendingEvents() recursively
             if (!isFlushingPendingEvents) {

and rewrite the "finally" block:

         } finally {
             if (!wasFlushing) {
                 isFlushingPendingEvents = false;
             }
             flushLock.unlock();
         }

But a better fix would be to get rid of the separate lock (flushLock) 
and use the one from EventQueue (pushPopLock). It's not as trivial as it 
seems to be, EventQueue and SunToolkit are in different packages, but 
definitely doable.

Thanks,

Artem

> Peter
>
> On Thursday, June 28, 2012 09:36:08 AM Peter Levart wrote:
>> While looking at the SunToolkit code I spoted another bug. The following
>> code:
>>
>>
>>      private static final Lock flushLock = new ReentrantLock();
>>      private static boolean isFlushingPendingEvents = false;
>>
>>      /*
>>       * Flush any pending events which haven't been posted to the AWT
>>       * EventQueue yet.
>>       */
>>      public static void flushPendingEvents()  {
>>          flushLock.lock();
>>          try {
>>              // Don't call flushPendingEvents() recursively
>>              if (!isFlushingPendingEvents) {
>>                  isFlushingPendingEvents = true;
>>                  AppContext appContext = AppContext.getAppContext();
>>                  PostEventQueue postEventQueue =
>>                      (PostEventQueue)appContext.get(POST_EVENT_QUEUE_KEY);
>>                  if (postEventQueue != null) {
>>                      postEventQueue.flush();
>>                  }
>>              }
>>          } finally {
>>              isFlushingPendingEvents = false;
>>              flushLock.unlock();
>>          }
>>      }
>>
>>
>> ... is clearly wrong. The isFlushingPendingEvents flag is reset in finally
>> block regardless of whether it was true or false at the entry to the try
>> block.
>>
>> The 1st nested call to flushPendingEvents() prevents recursion but it also
>> resets the flag so that the second nested call is allowed...
>>
>>
>>
>> Regards, Peter



More information about the awt-dev mailing list