Re-working of fix for POOL-303 git-svn-id: https://svn.apache.org/repos/asf/commons/proper/pool/trunk@1735269 13f79535-47bb-0310-9956-ffa450edef68
Project: http://git-wip-us.apache.org/repos/asf/commons-pool/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-pool/commit/a4c544a2 Tree: http://git-wip-us.apache.org/repos/asf/commons-pool/tree/a4c544a2 Diff: http://git-wip-us.apache.org/repos/asf/commons-pool/diff/a4c544a2 Branch: refs/heads/master Commit: a4c544a24242701673073d32d2ddbf037fac0099 Parents: 170a509 Author: Mark Thomas <ma...@apache.org> Authored: Wed Mar 16 17:20:41 2016 +0000 Committer: Mark Thomas <ma...@apache.org> Committed: Wed Mar 16 17:20:41 2016 +0000 ---------------------------------------------------------------------- .../pool2/impl/GenericKeyedObjectPool.java | 53 ++++++++++++++++---- .../commons/pool2/impl/GenericObjectPool.java | 51 ++++++++++++++++--- .../pool2/impl/TestGenericObjectPool.java | 6 ++- 3 files changed, 93 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-pool/blob/a4c544a2/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java index dcfe448..d477cfa 100644 --- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java @@ -1017,26 +1017,58 @@ public class GenericKeyedObjectPool<K,T> extends BaseGenericObjectPool<T> } } - final long newCreateCount = objectDeque.getCreateCount().incrementAndGet(); + // Flag that indicates if create should: + // - TRUE: call the factory to create an object + // - FALSE: return null + // - null: loop and re-test the condition that determines whether to + // call the factory + Boolean create = null; + while (create == null) { + synchronized (objectDeque.makeObjectCountLock) { + final long newCreateCount = objectDeque.getCreateCount().incrementAndGet(); + // Check against the per key limit + if (newCreateCount > maxTotalPerKeySave) { + // The key is currently at capacity or in the process of + // making enough new objects to take it to capacity. + numTotal.decrementAndGet(); + objectDeque.getCreateCount().decrementAndGet(); + if (objectDeque.makeObjectCount == 0) { + // There are no makeObject() calls in progress for this + // key so the key is at capacity. Do not attempt to + // create a new object. Return and wait for an object to + // be returned. + create = Boolean.FALSE; + } else { + // There are makeObject() calls in progress that might + // bring the pool to capacity. Those calls might also + // fail so wait until they complete and then re-test if + // the pool is at capacity or not. + objectDeque.makeObjectCountLock.wait(); + } + } else { + // The pool is not at capacity. Create a new object. + objectDeque.makeObjectCount++; + create = Boolean.TRUE; + } + } + } - // Check against the per key limit - if (newCreateCount > maxTotalPerKeySave) { - numTotal.decrementAndGet(); - objectDeque.getCreateCount().decrementAndGet(); + if (!create.booleanValue()) { return null; } - PooledObject<T> p = null; try { p = factory.makeObject(key); } catch (final Exception e) { numTotal.decrementAndGet(); objectDeque.getCreateCount().decrementAndGet(); - // POOL-303. There may be threads waiting on an object return that - // isn't going to happen. Unblock them. - objectDeque.idleObjects.interuptTakeWaiters(); throw e; + } finally { + synchronized (objectDeque.makeObjectCountLock) { + objectDeque.makeObjectCount--; + objectDeque.makeObjectCountLock.notifyAll(); + } } createdCount.incrementAndGet(); @@ -1431,6 +1463,9 @@ public class GenericKeyedObjectPool<K,T> extends BaseGenericObjectPool<T> */ private final AtomicInteger createCount = new AtomicInteger(0); + private long makeObjectCount = 0; + private final Object makeObjectCountLock = new Object(); + /* * The map is keyed on pooled instances, wrapped to ensure that * they work properly as keys. http://git-wip-us.apache.org/repos/asf/commons-pool/blob/a4c544a2/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java index 0e0c9bd..487eec2 100644 --- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java @@ -842,24 +842,59 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> */ private PooledObject<T> create() throws Exception { int localMaxTotal = getMaxTotal(); + // This simplifies the code later in this method if (localMaxTotal < 0) { localMaxTotal = Integer.MAX_VALUE; } - final long newCreateCount = createCount.incrementAndGet(); - if (newCreateCount > localMaxTotal) { - createCount.decrementAndGet(); + + // Flag that indicates if create should: + // - TRUE: call the factory to create an object + // - FALSE: return null + // - null: loop and re-test the condition that determines whether to + // call the factory + Boolean create = null; + while (create == null) { + synchronized (makeObjectCountLock) { + final long newCreateCount = createCount.incrementAndGet(); + if (newCreateCount > localMaxTotal) { + // The pool is currently at capacity or in the process of + // making enough new objects to take it to capacity. + createCount.decrementAndGet(); + if (makeObjectCount == 0) { + // There are no makeObject() calls in progress so the + // pool is at capacity. Do not attempt to create a new + // object. Return and wait for an object to be returned + create = Boolean.FALSE; + } else { + // There are makeObject() calls in progress that might + // bring the pool to capacity. Those calls might also + // fail so wait until they complete and then re-test if + // the pool is at capacity or not. + makeObjectCountLock.wait(); + } + } else { + // The pool is not at capacity. Create a new object. + makeObjectCount++; + create = Boolean.TRUE; + } + } + } + + if (!create.booleanValue()) { return null; } final PooledObject<T> p; try { p = factory.makeObject(); - } catch (final Exception e) { + } catch (Exception e) { createCount.decrementAndGet(); - // POOL-303. There may be threads waiting on an object return that - // isn't going to happen. Unblock them. - idleObjects.interuptTakeWaiters(); throw e; + } finally { + synchronized (makeObjectCountLock) { + makeObjectCount--; + makeObjectCountLock.notifyAll(); + } } final AbandonedConfig ac = this.abandonedConfig; @@ -1129,6 +1164,8 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * {@link #_maxActive} objects created at any one time. */ private final AtomicLong createCount = new AtomicLong(0); + private long makeObjectCount = 0; + private final Object makeObjectCountLock = new Object(); private final LinkedBlockingDeque<PooledObject<T>> idleObjects; // JMX specific attributes http://git-wip-us.apache.org/repos/asf/commons-pool/blob/a4c544a2/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java index f951691..5890146 100644 --- a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java +++ b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.lang.management.ManagementFactory; +import java.nio.charset.UnsupportedCharsetException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -2590,6 +2591,9 @@ public class TestGenericObjectPool extends TestBaseObjectPool { } Assert.assertFalse(thread1.isAlive()); Assert.assertFalse(thread2.isAlive()); + + Assert.assertTrue(thread1._thrown instanceof UnsupportedCharsetException); + Assert.assertTrue(thread2._thrown instanceof UnsupportedCharsetException); } private static class CreateFailFactory extends BasePooledObjectFactory<String> { @@ -2599,7 +2603,7 @@ public class TestGenericObjectPool extends TestBaseObjectPool { @Override public String create() throws Exception { semaphore.acquire(); - throw new Exception(); + throw new UnsupportedCharsetException("wibble"); } @Override
