commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pste...@apache.org
Subject svn commit: r1661444 - in /commons/proper/pool/trunk/src: changes/ main/java/org/apache/commons/pool2/impl/ test/java/org/apache/commons/pool2/impl/
Date Sat, 21 Feb 2015 22:08:07 GMT
Author: psteitz
Date: Sat Feb 21 22:08:06 2015
New Revision: 1661444

URL: http://svn.apache.org/r1661444
Log:
Fixed GKOP evictor capacity leak.  JIRA: POOL-287. Reported by Caleb Spare.  Patched by Thomas
Neidhart.

Modified:
    commons/proper/pool/trunk/src/changes/changes.xml
    commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
    commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
    commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
    commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java

Modified: commons/proper/pool/trunk/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/changes/changes.xml?rev=1661444&r1=1661443&r2=1661444&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/changes/changes.xml (original)
+++ commons/proper/pool/trunk/src/changes/changes.xml Sat Feb 21 22:08:06 2015
@@ -44,6 +44,10 @@ The <action> type attribute can be add,u
   </properties>
   <body>
   <release version="2.4" date="TBD" description="TBD">
+    <action dev="psteitz" type="fix" issue="POOL-287" due-to="Caleb Spare and Thomas Neidhart">
+      Fixed capacity leak when an object is offered from a GenericKeyedObjectPool while it
is
+      being validated by the evictor. 
+    </action>
   </release>
   <release version="2.3" date="2014-12-30" description=
 "This is a maintenance release that includes bug fixes and minor enhancements.">  

Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java?rev=1661444&r1=1661443&r2=1661444&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
Sat Feb 21 22:08:06 2015
@@ -21,6 +21,7 @@ import java.io.StringWriter;
 import java.io.Writer;
 import java.lang.management.ManagementFactory;
 import java.lang.ref.WeakReference;
+import java.util.Deque;
 import java.util.Iterator;
 import java.util.TimerTask;
 import java.util.concurrent.atomic.AtomicLong;
@@ -91,7 +92,7 @@ public abstract class BaseGenericObjectP
     volatile boolean closed = false;
     final Object evictionLock = new Object();
     private Evictor evictor = null; // @GuardedBy("evictionLock")
-    Iterator<PooledObject<T>> evictionIterator = null; // @GuardedBy("evictionLock")
+    EvictionIterator evictionIterator = null; // @GuardedBy("evictionLock")
     /*
      * Class loader for evictor thread to use since, in a JavaEE or similar
      * environment, the context class loader for the evictor thread may not have
@@ -1100,4 +1101,55 @@ public abstract class BaseGenericObjectP
             return (long) result;
         }
     }
+    
+    /**
+     * The idle object eviction iterator. Holds a reference to the idle objects.
+     */
+    class EvictionIterator implements Iterator<PooledObject<T>> {
+
+        private final Deque<PooledObject<T>> idleObjects;
+        private final Iterator<PooledObject<T>> idleObjectIterator;
+
+        /**
+         * Create an EvictionIterator for the provided idle instance deque.
+         * @param idleObjects underlying deque
+         */
+        EvictionIterator(final Deque<PooledObject<T>> idleObjects) {
+            this.idleObjects = idleObjects;
+
+            if (getLifo()) {
+                idleObjectIterator = idleObjects.descendingIterator();
+            } else {
+                idleObjectIterator = idleObjects.iterator();
+            }
+        }
+
+        /**
+         * Returns the idle object deque referenced by this iterator.
+         * @return the idle object deque
+         */
+        public Deque<PooledObject<T>> getIdleObjects() {
+            return idleObjects;
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public boolean hasNext() {
+            return idleObjectIterator.hasNext();
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public PooledObject<T> next() {
+            return idleObjectIterator.next();
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public void remove() {
+            idleObjectIterator.remove();
+        }
+        
+    }
+
 }

Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=1661444&r1=1661443&r2=1661444&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
Sat Feb 21 22:08:06 2015
@@ -17,6 +17,7 @@
 package org.apache.commons.pool2.impl;
 
 import java.util.ArrayList;
+import java.util.Deque;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -879,8 +880,6 @@ public class GenericKeyedObjectPool<K,T>
 
             boolean testWhileIdle = getTestWhileIdle();
 
-            LinkedBlockingDeque<PooledObject<T>> idleObjects = null;
-
             for (int i = 0, m = getNumTests(); i < m; i++) {
                 if(evictionIterator == null || !evictionIterator.hasNext()) {
                     if (evictionKeyIterator == null ||
@@ -901,13 +900,9 @@ public class GenericKeyedObjectPool<K,T>
                         if (objectDeque == null) {
                             continue;
                         }
-                        idleObjects = objectDeque.getIdleObjects();
-
-                        if (getLifo()) {
-                            evictionIterator = idleObjects.descendingIterator();
-                        } else {
-                            evictionIterator = idleObjects.iterator();
-                        }
+                        
+                        final Deque<PooledObject<T>> idleObjects = objectDeque.getIdleObjects();
+                        evictionIterator = new EvictionIterator(idleObjects);
                         if (evictionIterator.hasNext()) {
                             break;
                         }
@@ -918,8 +913,10 @@ public class GenericKeyedObjectPool<K,T>
                     // Pools exhausted
                     return;
                 }
+                final Deque<PooledObject<T>> idleObjects;
                 try {
                     underTest = evictionIterator.next();
+                    idleObjects = evictionIterator.getIdleObjects();
                 } catch (NoSuchElementException nsee) {
                     // Object was borrowed in another thread
                     // Don't count this as an eviction test so reduce i;
@@ -965,14 +962,12 @@ public class GenericKeyedObjectPool<K,T>
                             destroyedByEvictorCount.incrementAndGet();
                         }
                         if (active) {
-                            if (!factory.validateObject(evictionKey,
-                                    underTest)) {
+                            if (!factory.validateObject(evictionKey, underTest)) {
                                 destroy(evictionKey, underTest, true);
                                 destroyedByEvictorCount.incrementAndGet();
                             } else {
                                 try {
-                                    factory.passivateObject(evictionKey,
-                                            underTest);
+                                    factory.passivateObject(evictionKey, underTest);
                                 } catch (Exception e) {
                                     destroy(evictionKey, underTest, true);
                                     destroyedByEvictorCount.incrementAndGet();
@@ -1493,6 +1488,7 @@ public class GenericKeyedObjectPool<K,T>
         public Map<S, PooledObject<S>> getAllObjects() {
             return allObjects;
         }
+
     }
 
     //--- configuration attributes ---------------------------------------------

Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1661444&r1=1661443&r2=1661444&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Sat Feb 21 22:08:06 2015
@@ -752,11 +752,7 @@ public class GenericObjectPool<T> extend
 
                 for (int i = 0, m = getNumTests(); i < m; i++) {
                     if (evictionIterator == null || !evictionIterator.hasNext()) {
-                        if (getLifo()) {
-                            evictionIterator = idleObjects.descendingIterator();
-                        } else {
-                            evictionIterator = idleObjects.iterator();
-                        }
+                        evictionIterator = new EvictionIterator(idleObjects);
                     }
                     if (!evictionIterator.hasNext()) {
                         // Pool exhausted, nothing to do here

Modified: commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java?rev=1661444&r1=1661443&r2=1661444&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
Sat Feb 21 22:08:06 2015
@@ -1848,6 +1848,7 @@ public class TestGenericKeyedObjectPool
         }
         @Override
         public boolean validateObject(K key, PooledObject<String> obj) {
+            doWait(validateLatency);
             if (enableValidation) {
                 return validateCounter++%2 == 0 ? evenValid : oddValid;
             } else {
@@ -1878,6 +1879,9 @@ public class TestGenericKeyedObjectPool
         public void setMakeLatency(long makeLatency) {
             this.makeLatency = makeLatency;
         }
+        public void setValidateLatency(long validateLatency) {
+            this.validateLatency = validateLatency;
+        }
         public void setValidationEnabled(boolean b) {
             enableValidation = b;
         }
@@ -1911,6 +1915,7 @@ public class TestGenericKeyedObjectPool
         boolean enableValidation = false;
         long destroyLatency = 0;
         long makeLatency = 0;
+        long validateLatency = 0;
         volatile int maxTotalPerKey = Integer.MAX_VALUE;
         boolean exceptionOnPassivate = false;
         boolean exceptionOnActivate = false;
@@ -2136,6 +2141,41 @@ public class TestGenericKeyedObjectPool
         factory.exceptionOnCreate = false;
         pool.borrowObject("One");
     }
+    
+    /**
+     * JIRA: POOL-287
+     * 
+     * Verify that when an attempt is made to borrow an instance from the pool
+     * while the evictor is visiting it, there is no capacity leak.
+     * 
+     * Test creates the scenario described in POOL-287.
+     */
+    @Test
+    public void testReturnToHead() throws Exception {
+        SimpleFactory<String> factory = new SimpleFactory<String>();
+        factory.setValidateLatency(100);
+        factory.setValid(true);  // Validation always succeeds
+        GenericKeyedObjectPool<String, String> pool = new GenericKeyedObjectPool<String,
String>(factory);
+        pool.setMaxWaitMillis(1000);
+        pool.setTestWhileIdle(true);
+        pool.setMaxTotalPerKey(2);
+        pool.setNumTestsPerEvictionRun(1);
+        pool.setTimeBetweenEvictionRunsMillis(500);
+        
+        // Load pool with two objects
+        pool.addObject("one");  // call this o1
+        pool.addObject("one");  // call this o2
+        // Default is LIFO, so "one" pool is now [o2, o1] in offer order.
+        // Evictor will visit in oldest-to-youngest order, so o1 then o2
+        
+        Thread.sleep(800); // Wait for first eviction run to complete
+        
+        // At this point, one eviction run should have completed, visiting o1
+        // and eviction cursor should be pointed at o2, which is the next offered instance
+        Thread.sleep(250);         // Wait for evictor to start
+        pool.borrowObject("one");  // o2 is under eviction, so this will return o1
+        pool.borrowObject("one");  // Once validation completes, o2 should be offered
+    }
 
     private static class DummyFactory
             extends BaseKeyedPooledObjectFactory<Object,Object> {



Mime
View raw message