You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ps...@apache.org on 2015/02/21 23:08:07 UTC
svn commit: r1661444 - in /commons/proper/pool/trunk/src: changes/
main/java/org/apache/commons/pool2/impl/
test/java/org/apache/commons/pool2/impl/
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> {