Should this be ported to the 1.6 branch? Gary
On Sat, Dec 29, 2012 at 1:23 PM, <pste...@apache.org> wrote: > Author: psteitz > Date: Sat Dec 29 18:23:33 2012 > New Revision: 1426799 > > URL: http://svn.apache.org/viewvc?rev=1426799&view=rev > Log: > Fixed threadsafety problem in GOP, GKOP invalidateObject. JIRA: POOL-231. > > Modified: > > 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 > > commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java > > 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=1426799&r1=1426798&r2=1426799&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 Dec 29 18:23:33 2012 > @@ -551,7 +551,11 @@ public class GenericKeyedObjectPool<K,T> > throw new IllegalStateException( > "Object not currently part of this pool"); > } > - destroy(key, p, true); > + synchronized (p) { > + if (p.getState() != PooledObjectState.INVALID) { > + destroy(key, p, true); > + } > + } > } > > > > 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=1426799&r1=1426798&r2=1426799&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 Dec 29 18:23:33 2012 > @@ -581,10 +581,14 @@ public class GenericObjectPool<T> extend > return; > } else { > throw new IllegalStateException( > - "Returned object not currently part of this > pool"); > + "Invalidated object not currently part of this > pool"); > } > } > - destroy(p); > + synchronized (p) { > + if (p.getState() != PooledObjectState.INVALID) { > + destroy(p); > + } > + } > } > > /** > > 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=1426799&r1=1426798&r2=1426799&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 Dec 29 18:23:33 2012 > @@ -1433,6 +1433,88 @@ public class TestGenericKeyedObjectPool > // Check thread was interrupted > assertTrue(wtt._thrown instanceof InterruptedException); > } > + > + /** > + * POOL-231 - verify that concurrent invalidates of the same object > do not > + * corrupt pool destroyCount. > + */ > + @Test > + public void testConcurrentInvalidate() throws Exception { > + // Get allObjects and idleObjects loaded with some instances > + final int nObjects = 1000; > + final String key = "one"; > + pool.setMaxTotal(nObjects); > + pool.setMaxTotalPerKey(nObjects); > + pool.setMaxIdlePerKey(nObjects); > + final String [] obj = new String[nObjects]; > + for (int i = 0; i < nObjects; i++) { > + obj[i] = pool.borrowObject(key); > + } > + for (int i = 0; i < nObjects; i++) { > + if (i % 2 == 0) { > + pool.returnObject(key, obj[i]); > + } > + } > + final int nThreads = 20; > + final int nIterations = 60; > + final InvalidateThread[] threads = new InvalidateThread[nThreads]; > + // Randomly generated list of distinct invalidation targets > + final ArrayList<Integer> targets = new ArrayList<Integer>(); > + final Random random = new Random(); > + for (int j = 0; j < nIterations; j++) { > + // Get a random invalidation target > + Integer targ = new Integer(random.nextInt(nObjects)); > + while (targets.contains(targ)) { > + targ = new Integer(random.nextInt(nObjects)); > + } > + targets.add(targ); > + // Launch nThreads threads all trying to invalidate the target > + for (int i = 0; i < nThreads; i++) { > + threads[i] = new InvalidateThread(pool,key, obj[targ]); > + } > + for (int i = 0; i < nThreads; i++) { > + new Thread(threads[i]).start(); > + } > + boolean done = false; > + while (!done) { > + done = true; > + for (int i = 0; i < nThreads; i++) { > + done = done && threads[i].complete(); > + } > + Thread.sleep(100); > + } > + } > + Assert.assertEquals(nIterations, pool.getDestroyedCount()); > + } > + > + /** > + * Attempts to invalidate an object, swallowing IllegalStateException. > + */ > + static class InvalidateThread implements Runnable { > + private final String obj; > + private final KeyedObjectPool<String, String> pool; > + private final String key; > + private boolean done = false; > + public InvalidateThread(KeyedObjectPool<String, String> pool, > String key, String obj) { > + this.obj = obj; > + this.pool = pool; > + this.key = key; > + } > + public void run() { > + try { > + pool.invalidateObject(key, obj); > + } catch (IllegalStateException ex) { > + // Ignore > + } catch (Exception ex) { > + Assert.fail("Unexpected exception " + ex.toString()); > + } finally { > + done = true; > + } > + } > + public boolean complete() { > + return done; > + } > + } > > /* > * Very simple test thread that just tries to borrow an object from > > 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=1426799&r1=1426798&r2=1426799&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 > Sat Dec 29 18:23:33 2012 > @@ -1100,6 +1100,84 @@ public class TestGenericObjectPool exten > assertEquals("Total count different than expected.", 0, > pool.getNumActive()); > pool.close(); > } > + > + /** > + * POOL-231 - verify that concurrent invalidates of the same object > do not > + * corrupt pool destroyCount. > + */ > + @Test > + public void testConcurrentInvalidate() throws Exception { > + // Get allObjects and idleObjects loaded with some instances > + final int nObjects = 1000; > + pool.setMaxTotal(nObjects); > + pool.setMaxIdle(nObjects); > + final Object[] obj = new Object[nObjects]; > + for (int i = 0; i < nObjects; i++) { > + obj[i] = pool.borrowObject(); > + } > + for (int i = 0; i < nObjects; i++) { > + if (i % 2 == 0) { > + pool.returnObject(obj[i]); > + } > + } > + final int nThreads = 20; > + final int nIterations = 60; > + final InvalidateThread[] threads = new InvalidateThread[nThreads]; > + // Randomly generated list of distinct invalidation targets > + final ArrayList<Integer> targets = new ArrayList<Integer>(); > + final Random random = new Random(); > + for (int j = 0; j < nIterations; j++) { > + // Get a random invalidation target > + Integer targ = new Integer(random.nextInt(nObjects)); > + while (targets.contains(targ)) { > + targ = new Integer(random.nextInt(nObjects)); > + } > + targets.add(targ); > + // Launch nThreads threads all trying to invalidate the target > + for (int i = 0; i < nThreads; i++) { > + threads[i] = new InvalidateThread(pool, obj[targ]); > + } > + for (int i = 0; i < nThreads; i++) { > + new Thread(threads[i]).start(); > + } > + boolean done = false; > + while (!done) { > + done = true; > + for (int i = 0; i < nThreads; i++) { > + done = done && threads[i].complete(); > + } > + Thread.sleep(100); > + } > + } > + Assert.assertEquals(nIterations, pool.getDestroyedCount()); > + } > + > + /** > + * Attempts to invalidate an object, swallowing IllegalStateException. > + */ > + static class InvalidateThread implements Runnable { > + private final Object obj; > + private final ObjectPool<Object> pool; > + private boolean done = false; > + public InvalidateThread(ObjectPool<Object> pool, Object obj) { > + this.obj = obj; > + this.pool = pool; > + } > + public void run() { > + try { > + pool.invalidateObject(obj); > + } catch (IllegalStateException ex) { > + // Ignore > + } catch (Exception ex) { > + Assert.fail("Unexpected exception " + ex.toString()); > + } finally { > + done = true; > + } > + } > + public boolean complete() { > + return done; > + } > + } > > @Test(timeout=60000) > public void testMinIdle() throws Exception { > > > -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0 Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
