On 1/10/13 8:24 AM, Mark Thomas wrote: > On 10/01/2013 16:21, Gary Gregory wrote: >> Should this be ported to the 1.6 branch? > Don't think so. That looks to be specific to pool2.
Right. In pool 1, multiple threads trying to invalidate the same object violates the pool contract and adding sync protection won't help. Serializing the repeated invalidates still results in pool counters borked. This is a 1.x feature (multiple invalidates or returns on the same object violate the contract and when you do this, there is no guarantee the pool counters are going to be correct). In pool 2, multiple invalidates cause an exception and the pool counters maintain integrity. The fix below just makes sure that when multiple threads try to concurrently invalidate an object, at most one of them succeeds and the pool counters are updated only once. Looking carefully again at the fix, I realize now that while it does ensure counter integrity, it does not ensure that multiple invalidates always throw. It might actually be best to add an else that throws in the sync block. The javadoc should also probably be a little more explicit about what the contract is. What do others think? Phil > > Mark > > >> Gary >> >> >> On Sat, Dec 29, 2012 at 1:23 PM, <[email protected]> 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 { >>> >>> >>> >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
