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.
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]
