On 7/9/14, 8:42 PM, Phil Steitz wrote:
> On 7/9/14, 2:29 PM, [email protected] wrote:
>> 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);
>> + }
>> }
> Another option here might be to modify LinkedBlockingDeque to
> support close() semantics and have close on the pool close the deque
> (so addLast/First would return false on a closed deque and
> returnObject would just destroy the one object instead of having to
> add it back and then clear it). I did some benchmarking a while
> back to see if ripping out the capacity checking made any difference
> to performance and was surprised to find that I could not measure
> this. A closed check might similarly have minimal performance
> impact; or at least swapping out the needless capacity check for a
> closed check might be net zero.
>>
>> 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.
> I played with this a little and unfortunately I think you are
> right. There is no way to make this happen reliably with the
> current code.
This is a little smelly, but one way to rig the test would be to
define a GOP subclass in the test that overrides getLifo to include
a delay. That makes the test depend (for catching this bug) on
current impl, but I doubt the detail it depends on is likely to
change any time soon. Given the proximity of calls to this method
to deque access in other places, introducing a delay might catch
other bugs too.
Phil
>
> Phil
>> + */
>> + //@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
>>
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
