[POOL-386] Closing a pool can cause Evictor in another pool to be cancelled

• Type: Task
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 2.6.1, 2.6.2, 2.7.0, 2.8.0
• Fix Version/s: 2.8.1
• External issue URL: https://github.com/apache/commons-pool/pull/32
• Assignee: Unassigned
• Reporter: Phil Steitz
• Created: 09/Jul/20 02:18
• Updated: 25/Jul/20 03:08
• Resolved: 25/Jul/20 03:08

Description

The fix for POOL-337 introduced a race condition that can cause the shared EvictionTimer to be cancelled when a pool is closed but another pool still has an active Evictor.  The EvictionTimer cancel method used to cancel an eviction task owned by a client pool uses this test to determine whether or not to shutdown its executor:

 if (executor != null && executor.getQueue().isEmpty())

The executor may report an empty queue if it is executing a task.  This will cause the executor to be shut down and scheduling of new tasks to stop.

The unit test below illustrates the problem.

 
public void testEvictionTimerMultiplePools() throws InterruptedException {
        final AtomicIntegerFactory factory = new AtomicIntegerFactory();
        factory.setValidateLatency(50);
        final GenericObjectPool<AtomicInteger> evictingPool = new GenericObjectPool<>(factory);
        evictingPool.setTimeBetweenEvictionRunsMillis(100);
        evictingPool.setNumTestsPerEvictionRun(5);
        evictingPool.setTestWhileIdle(true);
        evictingPool.setMinEvictableIdleTimeMillis(50);
        for (int i = 0; i < 10; i++) {
            try {
                evictingPool.addObject();
            } catch (Exception e) {
                e.printStackTrace();
            }
        }


        for (int i = 0; i < 1000; i++) {
            GenericObjectPool<AtomicInteger> nonEvictingPool = new GenericObjectPool<>(factory);
            nonEvictingPool.close();
        }


        Thread.sleep(1000);
        Assert.assertEquals(0, evictingPool.getNumIdle());
        evictingPool.close();
    }

Proposed fix:

Change the test guarding executor shutdown to  

 executor.getQueue().isEmpty() && executor.getActiveCount() == 0

This still exposes a low-probability race - a task completes after the isEmpty test and is requeued before the activecount test - but this is very unlikely.