Author: markt
Date: Wed Jul 9 21:29:44 2014
New Revision: 1609308
URL: http://svn.apache.org/r1609308
Log:
POOL-263
Fix a threading issue that meant that concurrent calls to close() and
returnObject() could result in some returned objects not being destroyed.
Modified:
commons/proper/pool/trunk/src/changes/changes.xml
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/TestGenericObjectPool.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=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/changes/changes.xml (original)
+++ commons/proper/pool/trunk/src/changes/changes.xml Wed Jul 9 21:29:44 2014
@@ -45,6 +45,10 @@ The <action> type attribute can be add,u
<body>
<release version="2.3" date="TBD" description=
"TBD">
+ <action dev="markt" type="fix" issue="POOL-263">
+ Fix a threading issue that meant that concurrent calls to close() and
+ returnObject() could result in some returned objects not being destroyed.
+ </action>
<action dev="psteitz" type="add" issue="POOL-262">
Made fairness configurable for GenericObjectPool, GenericKeyedObjectPool.
</action>
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=1609308&r1=1609307&r2=1609308&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
Wed Jul 9 21:29:44 2014
@@ -535,6 +535,12 @@ public class GenericKeyedObjectPool<K,T>
} else {
idleObjects.addLast(p);
}
+ if (isClosed()) {
+ // Pool closed while object was being added to idle objects.
+ // Make sure the returned object is destroyed rather than left
+ // in the idle object pool (which would effectively be a leak)
+ clear(key);
+ }
}
if (hasBorrowWaiters()) {
@@ -1430,9 +1436,8 @@ public class GenericKeyedObjectPool<K,T>
*/
public ObjectDeque(boolean fairness) {
idleObjects = new LinkedBlockingDeque<PooledObject<S>>(fairness);
-
}
-
+
/**
* Obtain the idle objects for the current key.
*
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=1609308&r1=1609307&r2=1609308&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
Wed Jul 9 21:29:44 2014
@@ -111,7 +111,7 @@ public class GenericObjectPool<T> extend
throw new IllegalArgumentException("factory may not be null");
}
this.factory = factory;
-
+
idleObjects = new
LinkedBlockingDeque<PooledObject<T>>(config.getFairness());
setConfig(config);
@@ -609,6 +609,12 @@ public class GenericObjectPool<T> extend
} else {
idleObjects.addLast(p);
}
+ if (isClosed()) {
+ // Pool closed while object was being added to idle objects.
+ // Make sure the returned object is destroyed rather than left
+ // in the idle object pool (which would effectively be a leak)
+ clear();
+ }
}
updateStatsReturn(activeTime);
}
@@ -904,6 +910,12 @@ public class GenericObjectPool<T> extend
idleObjects.addLast(p);
}
}
+ if (isClosed()) {
+ // Pool closed while object was being added to idle objects.
+ // Make sure the returned object is destroyed rather than left
+ // in the idle object pool (which would effectively be a leak)
+ clear();
+ }
}
/**
Modified:
commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
URL:
http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java?rev=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
---
commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
(original)
+++
commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
Wed Jul 9 21:29:44 2014
@@ -754,6 +754,45 @@ public class TestGenericObjectPool exten
}
}
+ /**
+ * This is the test case for POOL-263. It is disabled since it will always
+ * pass without artificial delay being injected into GOP.returnObject() and
+ * a way to this this hasn't currently been found that doesn't involve
+ * polluting the GOP implementation. The artificial delay needs to be
+ * inserted just before the final call to isLifo() in the returnObject()
+ * method.
+ */
+ //@Test(timeout=60000)
+ public void testReturnObject() throws Exception {
+
+ pool.setMaxTotal(1);
+ pool.setMaxIdle(-1);
+ String active = pool.borrowObject();
+
+ assertEquals(1, pool.getNumActive());
+ assertEquals(0, pool.getNumIdle());
+
+ Thread t = new Thread() {
+
+ @Override
+ public void run() {
+ pool.close();
+ }
+
+ };
+ t.start();
+
+ pool.returnObject(active);
+
+ // Wait for the close() thread to complete
+ while (t.isAlive()) {
+ Thread.sleep(50);
+ }
+
+ assertEquals(0, pool.getNumIdle());
+ }
+
+
@Test(timeout=60000)
public void testSettersAndGetters() throws Exception {
{
@@ -1396,7 +1435,7 @@ public class TestGenericObjectPool exten
*/
}
}
-
+
/**
* Verifies that concurrent threads never "share" instances
*/
@@ -1516,7 +1555,7 @@ public class TestGenericObjectPool exten
boolean randomDelay, Object obj) {
this(pool, iter, delay, delay, randomDelay, obj);
}
-
+
public TestThread(ObjectPool<T> pool, int iter, int startDelay,
int holdTime, boolean randomDelay, Object obj) {
_pool = pool;
@@ -1827,7 +1866,7 @@ public class TestGenericObjectPool exten
}
}
}
-
+
protected static class AtomicIntegerFactory
extends BasePooledObjectFactory<AtomicInteger> {
@@ -1836,7 +1875,7 @@ public class TestGenericObjectPool exten
private long createLatency = 0;
private long destroyLatency = 0;
private long validateLatency = 0;
-
+
@Override
public AtomicInteger create() {
try {
@@ -1873,7 +1912,7 @@ public class TestGenericObjectPool exten
} catch (InterruptedException ex) {}
return instance.getObject().intValue() == 1;
}
-
+
@Override
public void destroyObject(PooledObject<AtomicInteger> p) {
try {
@@ -1881,7 +1920,7 @@ public class TestGenericObjectPool exten
} catch (InterruptedException ex) {}
}
-
+
/**
* @param activateLatency the activateLatency to set
*/
@@ -1889,7 +1928,7 @@ public class TestGenericObjectPool exten
this.activateLatency = activateLatency;
}
-
+
/**
* @param passivateLatency the passivateLatency to set
*/
@@ -1897,7 +1936,7 @@ public class TestGenericObjectPool exten
this.passivateLatency = passivateLatency;
}
-
+
/**
* @param createLatency the createLatency to set
*/
@@ -1905,7 +1944,7 @@ public class TestGenericObjectPool exten
this.createLatency = createLatency;
}
-
+
/**
* @param destroyLatency the destroyLatency to set
*/
@@ -1913,7 +1952,7 @@ public class TestGenericObjectPool exten
this.destroyLatency = destroyLatency;
}
-
+
/**
* @param validateLatency the validateLatency to set
*/
@@ -1942,7 +1981,7 @@ public class TestGenericObjectPool exten
})
@Test(timeout=60000)
public void testBorrowObjectFairness() throws Exception {
-
+
int numThreads = 40;
int maxTotal = 40;
@@ -1951,9 +1990,9 @@ public class TestGenericObjectPool exten
config.setMaxIdle(maxTotal);
config.setFairness(true);
config.setLifo(false);
-
+
pool = new GenericObjectPool(factory, config);
-
+
// Exhaust the pool
String[] objects = new String[maxTotal];
for (int i = 0; i < maxTotal; i++) {
@@ -1973,7 +2012,7 @@ public class TestGenericObjectPool exten
fail(e.toString());
}
}
-
+
// Return objects, other threads should get served in order
for (int i = 0; i < maxTotal; i++) {
pool.returnObject(objects[i]);
@@ -2310,7 +2349,7 @@ public class TestGenericObjectPool exten
Assert.assertEquals(0, pool.getNumActive());
Assert.assertEquals(1, pool.getNumIdle());
}
-
+
// POOL-259
@Test
public void testClientWaitStats() throws Exception {
@@ -2327,8 +2366,8 @@ public class TestGenericObjectPool exten
pool.borrowObject();
// Second borrow does not have to wait on create, average should be
about 50
Assert.assertTrue(pool.getMaxBorrowWaitTimeMillis() >= 100);
- Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() < 60);
- Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() > 40);
+ Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() < 60);
+ Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() > 40);
}
private static final class DummyFactory
