Uploaded image for project: 'Commons Pool'
  1. Commons Pool
  2. POOL-386

Closing a pool can cause Evictor in another pool to be cancelled

    XMLWordPrintableJSON

    Details

    • 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
    • Labels:
      None

      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.

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              psteitz Phil Steitz
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 4.5h
                4.5h