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

[Peter Levart]

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.

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 {
            // 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();
        }
    }


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