On 7/9/14, 2:29 PM, ma...@apache.org 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org