Fix POOL-310 Ensure that threads using GKOP do not block indefinitely if more than maxTotal threads try to borrow objects with different keys at the same time and the factory destroys objects on return.
git-svn-id: https://svn.apache.org/repos/asf/commons/proper/pool/trunk@1735563 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/9257fb9e Tree: http://git-wip-us.apache.org/repos/asf/commons-pool/tree/9257fb9e Diff: http://git-wip-us.apache.org/repos/asf/commons-pool/diff/9257fb9e Branch: refs/heads/master Commit: 9257fb9e40526f7ad33ac83f0b7f152607510ccd Parents: f7a0d26 Author: Mark Thomas <ma...@apache.org> Authored: Fri Mar 18 09:53:38 2016 +0000 Committer: Mark Thomas <ma...@apache.org> Committed: Fri Mar 18 09:53:38 2016 +0000 ---------------------------------------------------------------------- src/changes/changes.xml | 5 + .../pool2/impl/GenericKeyedObjectPool.java | 97 ++++++++++---------- 2 files changed, 53 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-pool/blob/9257fb9e/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 6b48a1f..65ca2b8 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -62,6 +62,11 @@ The <action> type attribute can be add,update,fix,remove. <action dev="markt" issue="POOL-307" type="update" due-to="Anthony Whitford"> Replace inefficient use of keySet with entrySet in GKOP. </action> + <action dev="markt" issue="POOL-310" type="fix" due-to="Ivan Iliev"> + Ensure that threads using GKOP do not block indefinitely if more than + maxTotal threads try to borrow objects with different keys at the same + time and the factory destroys objects on return. + </action> </release> <release version="2.4.2" date="2015-08-01" description= "This is a patch release, including bug fixes only."> http://git-wip-us.apache.org/repos/asf/commons-pool/blob/9257fb9e/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 200c424..6976f2a 100644 --- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java @@ -478,8 +478,29 @@ public class GenericKeyedObjectPool<K,T> extends BaseGenericObjectPool<T> final long activeTime = p.getActiveTimeMillis(); - if (getTestOnReturn()) { - if (!factory.validateObject(key, p)) { + try { + if (getTestOnReturn()) { + if (!factory.validateObject(key, p)) { + try { + destroy(key, p, true); + } catch (final Exception e) { + swallowException(e); + } + if (objectDeque.idleObjects.hasTakeWaiters()) { + try { + addObject(key); + } catch (final Exception e) { + swallowException(e); + } + } + return; + } + } + + try { + factory.passivateObject(key, p); + } catch (final Exception e1) { + swallowException(e1); try { destroy(key, p, true); } catch (final Exception e) { @@ -492,65 +513,43 @@ public class GenericKeyedObjectPool<K,T> extends BaseGenericObjectPool<T> swallowException(e); } } - updateStatsReturn(activeTime); return; } - } - try { - factory.passivateObject(key, p); - } catch (final Exception e1) { - swallowException(e1); - try { - destroy(key, p, true); - } catch (final Exception e) { - swallowException(e); + if (!p.deallocate()) { + throw new IllegalStateException( + "Object has already been returned to this pool"); } - if (objectDeque.idleObjects.hasTakeWaiters()) { + + final int maxIdle = getMaxIdlePerKey(); + final LinkedBlockingDeque<PooledObject<T>> idleObjects = + objectDeque.getIdleObjects(); + + if (isClosed() || maxIdle > -1 && maxIdle <= idleObjects.size()) { try { - addObject(key); + destroy(key, p, true); } catch (final Exception e) { swallowException(e); } - } - updateStatsReturn(activeTime); - return; - } - - if (!p.deallocate()) { - throw new IllegalStateException( - "Object has already been returned to this pool"); - } - - final int maxIdle = getMaxIdlePerKey(); - final LinkedBlockingDeque<PooledObject<T>> idleObjects = - objectDeque.getIdleObjects(); - - if (isClosed() || maxIdle > -1 && maxIdle <= idleObjects.size()) { - try { - destroy(key, p, true); - } catch (final Exception e) { - swallowException(e); - } - } else { - if (getLifo()) { - idleObjects.addFirst(p); } else { - idleObjects.addLast(p); + if (getLifo()) { + idleObjects.addFirst(p); + } 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); + } } - 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); + } finally { + if (hasBorrowWaiters()) { + reuseCapacity(); } + updateStatsReturn(activeTime); } - - if (hasBorrowWaiters()) { - reuseCapacity(); - } - - updateStatsReturn(activeTime); }