Thread deadlock in CacheEventQueue class

["Wyatt, Allen" <Al...@travelocity.com>]

I've encountered a deadlock in the org.apache.jcs.engine.CacheEventQueue
class.  One thread synchronizes on the queueLock object and then tries
synchronizing on the CacheEventQueue object itself while another thread
synchronizes on the CacheEventQueue object and then tries synchronizing
on the queueLock object.  Here are the stacks of the threads:

Thread #1:
----------
at org.apache.jcs.engine.CacheEventQueue.put(CacheEventQueue.java:299) -
waiting to lock <0x546eaac0> (a java.lang.Object (the queueLock object))
at
org.apache.jcs.engine.CacheEventQueue.addPutEvent(CacheEventQueue.java:2
11) - locked <0x546ea628> (a org.apache.jcs.engine.CacheEventQueue)
at
org.apache.jcs.auxiliary.disk.AbstractDiskCache.update(AbstractDiskCache
.java:148)
at
org.apache.jcs.engine.control.CompositeCache.spoolToDisk(CompositeCache.
java:346)
at
org.apache.jcs.engine.memory.AbstractMemoryCache.waterfal(AbstractMemory
Cache.java:230)
at
org.apache.jcs.engine.memory.shrinking.ShrinkerThread.shrink(ShrinkerThr
ead.java:247)
at
org.apache.jcs.engine.memory.shrinking.ShrinkerThread.run(ShrinkerThread
.java:119)

Thread #2 (running the Qprocessor inner class):
----------
at
org.apache.jcs.engine.CacheEventQueue.stopProcessing(CacheEventQueue.jav
a:126) - waiting to lock <0x546ea628> (a
org.apache.jcs.engine.CacheEventQueue)
at
org.apache.jcs.engine.CacheEventQueue$QProcessor.run(CacheEventQueue.jav
a:454) - locked <0x546eaac0> (a java.lang.Object (the queueLock object))

Is this a known problem?  Is there a fix?  I tried looking at
cvs.apache.org/viewcvs/jakarta-turbine-jcs/.../CacheEventQueue.java and
the code doesn't have a fix as far as I can tell.

I was thinking this could be fixed by changing the code in the
QProcessor inner class's run() method from:

    public void run()
    {
        AbstractCacheEvent r = null;

        while ( queue.isAlive() )
        {
            r = queue.take();
    
            if ( log.isDebugEnabled() )
            {
                log.debug( "Event from queue = " + r );
            }

            if ( r == null )
            {
                synchronized ( queueLock )
                {
                    try
                    {
                        queueLock.wait( queue.getWaitToDieMillis() );
                    }
                    catch ( InterruptedException e )
                    {
                        log.warn(
                            "Interrupted while waiting for another event
to come in before we die." );
                        return;
                    }
                    r = queue.take();
                    if ( log.isDebugEnabled() )
                    {
                        log.debug( "Event from queue after sleep = " + r
);
                    }
                    if ( r == null )
                    {
                        queue.stopProcessing();
                    }
                }
            }

            if ( queue.isWorking() && queue.isAlive() && r != null )
            {
                r.run();
            }
        }
        if ( log.isInfoEnabled() )
        {
            log.info( "QProcessor exiting for " + queue );
        }
    }

to the following code:

    public void run()
    {
        AbstractCacheEvent r = null;

        while ( queue.isAlive() )
        {
            r = queue.take();
    
            if ( log.isDebugEnabled() )
            {
                log.debug( "Event from queue = " + r );
            }

            if ( r == null )
            {
                synchronized ( queueLock )
                {
                    try
                    {
                        queueLock.wait( queue.getWaitToDieMillis() );
                    }
                    catch ( InterruptedException e )
                    {
                        log.warn(
                            "Interrupted while waiting for another event
to come in before we die." );
                        return;
                    }
                    r = queue.take();
                    if ( log.isDebugEnabled() )
                    {
                        log.debug( "Event from queue after sleep = " + r
);
                    }
                    /*** MOVED CODE FROM HERE (inside synchronized
block) TO BELOW (outside synchronized block) ***/
                }
                /*** MOVED CODE STARTS BELOW: ****/
                if ( r == null )
                {
                    queue.stopProcessing();
                }
                /*** END OF MOVED CODE ****/
            }

            if ( queue.isWorking() && queue.isAlive() && r != null )
            {
                r.run();
            }
        }
        if ( log.isInfoEnabled() )
        {
            log.info( "QProcessor exiting for " + queue );
        }
    }

Does this sound reasonable?