Author: markt
Date: Wed Dec 14 22:15:50 2011
New Revision: 1214507
URL: http://svn.apache.org/viewvc?rev=1214507&view=rev
Log:
Fix POOL-199. Ensure only one thread can call evict() at a time. Use a
dedicated (more fine-grained) lock rather than locking on the object.
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
URL:
http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=1214507&r1=1214506&r2=1214507&view=diff
==============================================================================
---
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
(original)
+++
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
Wed Dec 14 22:15:50 2011
@@ -1280,97 +1280,99 @@ public class GenericKeyedObjectPool<K,T>
return;
}
- boolean testWhileIdle = getTestWhileIdle();
- long idleEvictTime = Long.MAX_VALUE;
-
- if (getMinEvictableIdleTimeMillis() > 0) {
- idleEvictTime = getMinEvictableIdleTimeMillis();
- }
-
- PooledObject<T> underTest = null;
- LinkedBlockingDeque<PooledObject<T>> idleObjects = null;
-
- for (int i = 0, m = getNumTests(); i < m; i++) {
- if(evictionIterator == null || !evictionIterator.hasNext()) {
- if (evictionKeyIterator == null ||
- !evictionKeyIterator.hasNext()) {
- List<K> keyCopy = new ArrayList<K>();
- keyCopy.addAll(poolKeyList);
- evictionKeyIterator = keyCopy.iterator();
- }
- while (evictionKeyIterator.hasNext()) {
- evictionKey = evictionKeyIterator.next();
- ObjectDeque<T> objectDeque = poolMap.get(evictionKey);
- if (objectDeque == null) {
- continue;
- }
- idleObjects = objectDeque.getIdleObjects();
-
- if (getLifo()) {
- evictionIterator = idleObjects.descendingIterator();
- } else {
- evictionIterator = idleObjects.iterator();
+ synchronized (evictionLock) {
+ boolean testWhileIdle = getTestWhileIdle();
+ long idleEvictTime = Long.MAX_VALUE;
+
+ if (getMinEvictableIdleTimeMillis() > 0) {
+ idleEvictTime = getMinEvictableIdleTimeMillis();
+ }
+
+ PooledObject<T> underTest = null;
+ LinkedBlockingDeque<PooledObject<T>> idleObjects = null;
+
+ for (int i = 0, m = getNumTests(); i < m; i++) {
+ if(evictionIterator == null || !evictionIterator.hasNext()) {
+ if (evictionKeyIterator == null ||
+ !evictionKeyIterator.hasNext()) {
+ List<K> keyCopy = new ArrayList<K>();
+ keyCopy.addAll(poolKeyList);
+ evictionKeyIterator = keyCopy.iterator();
}
- if (evictionIterator.hasNext()) {
- break;
+ while (evictionKeyIterator.hasNext()) {
+ evictionKey = evictionKeyIterator.next();
+ ObjectDeque<T> objectDeque = poolMap.get(evictionKey);
+ if (objectDeque == null) {
+ continue;
+ }
+ idleObjects = objectDeque.getIdleObjects();
+
+ if (getLifo()) {
+ evictionIterator =
idleObjects.descendingIterator();
+ } else {
+ evictionIterator = idleObjects.iterator();
+ }
+ if (evictionIterator.hasNext()) {
+ break;
+ }
+ evictionIterator = null;
}
+ }
+ if (evictionIterator == null) {
+ // Pools exhausted
+ return;
+ }
+ try {
+ underTest = evictionIterator.next();
+ } catch (NoSuchElementException nsee) {
+ // Object was borrowed in another thread
+ // Don't count this as an eviction test so reduce i;
+ i--;
evictionIterator = null;
+ continue;
}
- }
- if (evictionIterator == null) {
- // Pools exhausted
- return;
- }
- try {
- underTest = evictionIterator.next();
- } catch (NoSuchElementException nsee) {
- // Object was borrowed in another thread
- // Don't count this as an eviction test so reduce i;
- i--;
- evictionIterator = null;
- continue;
- }
-
- if (!underTest.startEvictionTest()) {
- // Object was borrowed in another thread
- // Don't count this as an eviction test so reduce i;
- i--;
- continue;
- }
-
- if (idleEvictTime < underTest.getIdleTimeMillis()) {
- destroy(evictionKey, underTest, true);
- destroyedByEvictorCount.incrementAndGet();
- } else {
- if (testWhileIdle) {
- boolean active = false;
- try {
- factory.activateObject(evictionKey,
- underTest.getObject());
- active = true;
- } catch (Exception e) {
- destroy(evictionKey, underTest, true);
- destroyedByEvictorCount.incrementAndGet();
- }
- if (active) {
- if (!factory.validateObject(evictionKey,
- underTest.getObject())) {
+
+ if (!underTest.startEvictionTest()) {
+ // Object was borrowed in another thread
+ // Don't count this as an eviction test so reduce i;
+ i--;
+ continue;
+ }
+
+ if (idleEvictTime < underTest.getIdleTimeMillis()) {
+ destroy(evictionKey, underTest, true);
+ destroyedByEvictorCount.incrementAndGet();
+ } else {
+ if (testWhileIdle) {
+ boolean active = false;
+ try {
+ factory.activateObject(evictionKey,
+ underTest.getObject());
+ active = true;
+ } catch (Exception e) {
destroy(evictionKey, underTest, true);
destroyedByEvictorCount.incrementAndGet();
- } else {
- try {
- factory.passivateObject(evictionKey,
- underTest.getObject());
- } catch (Exception e) {
+ }
+ if (active) {
+ if (!factory.validateObject(evictionKey,
+ underTest.getObject())) {
destroy(evictionKey, underTest, true);
destroyedByEvictorCount.incrementAndGet();
+ } else {
+ try {
+ factory.passivateObject(evictionKey,
+ underTest.getObject());
+ } catch (Exception e) {
+ destroy(evictionKey, underTest, true);
+ destroyedByEvictorCount.incrementAndGet();
+ }
}
}
}
- }
- if (!underTest.endEvictionTest(idleObjects)) {
- // TODO - May need to add code here once additional states
- // are used
+ if (!underTest.endEvictionTest(idleObjects)) {
+ // TODO - May need to add code here once additional
states
+ // are used
+ }
}
}
}
@@ -1638,14 +1640,16 @@ public class GenericKeyedObjectPool<K,T>
* @param delay milliseconds between evictor runs.
*/
// Needs to be final; see POOL-195. Make protected method final as it is
called from constructor.
- protected final synchronized void startEvictor(long delay) {
- if (null != evictor) {
- EvictionTimer.cancel(evictor);
- evictor = null;
- }
- if (delay > 0) {
- evictor = new Evictor();
- EvictionTimer.schedule(evictor, delay, delay);
+ protected final void startEvictor(long delay) {
+ synchronized (evictionLock) {
+ if (null != evictor) {
+ EvictionTimer.cancel(evictor);
+ evictor = null;
+ }
+ if (delay > 0) {
+ evictor = new Evictor();
+ EvictionTimer.schedule(evictor, delay, delay);
+ }
}
}
@@ -2062,11 +2066,6 @@ public class GenericKeyedObjectPool<K,T>
/** My {@link KeyedPoolableObjectFactory}. */
final private KeyedPoolableObjectFactory<K,T> factory;
- /**
- * My idle object eviction {@link TimerTask}, if any.
- */
- private Evictor evictor = null; // @GuardedBy("this")
-
/** My hash of pools (ObjectQueue). */
private final Map<K,ObjectDeque<T>> poolMap =
new ConcurrentHashMap<K,ObjectDeque<T>>();
@@ -2086,21 +2085,29 @@ public class GenericKeyedObjectPool<K,T>
private final AtomicInteger numTotal = new AtomicInteger(0);
/**
+ * My idle object eviction {@link TimerTask}, if any.
+ */
+ private Evictor evictor = null; // @GuardedBy("evictionLock")
+
+ /**
* An iterator for {@link ObjectDeque#getIdleObjects()} that is used by the
* evictor.
*/
- private Iterator<PooledObject<T>> evictionIterator = null;
+ private Iterator<PooledObject<T>> evictionIterator = null; //
@GuardedBy("evictionLock") - except close()
/**
* An iterator for {@link #poolMap} entries.
*/
- private Iterator<K> evictionKeyIterator = null;
+ private Iterator<K> evictionKeyIterator = null; //
@GuardedBy("evictionLock") - except close()
/**
* The key associated with the {@link ObjectDeque#getIdleObjects()}
* currently being evicted.
*/
- private K evictionKey = null;
+ private K evictionKey = null; // @GuardedBy("evictionLock")
+
+ /** Object used to ensure thread safety of eviction process */
+ private final Object evictionLock = new Object();
/** Object used to ensure closed() is only called once. */
private final Object closeLock = new Object();
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
URL:
http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1214507&r1=1214506&r2=1214507&view=diff
==============================================================================
---
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
(original)
+++
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Wed Dec 14 22:15:50 2011
@@ -1044,84 +1044,84 @@ public class GenericObjectPool<T> extend
PooledObject<T> underTest = null;
- boolean testWhileIdle = getTestWhileIdle();
- long idleEvictTime = Long.MAX_VALUE;
- long idleSoftEvictTime = Long.MAX_VALUE;
-
- if (getMinEvictableIdleTimeMillis() > 0) {
- idleEvictTime = getMinEvictableIdleTimeMillis();
- }
- if (getSoftMinEvictableIdleTimeMillis() > 0) {
- idleSoftEvictTime = getSoftMinEvictableIdleTimeMillis();
- }
-
- for (int i = 0, m = getNumTests(); i < m; i++) {
- if (evictionIterator == null || !evictionIterator.hasNext()) {
- if (getLifo()) {
- evictionIterator = idleObjects.descendingIterator();
- } else {
- evictionIterator = idleObjects.iterator();
- }
- }
- if (!evictionIterator.hasNext()) {
- // Pool exhausted, nothing to do here
- return;
- }
-
- try {
- underTest = evictionIterator.next();
- } catch (NoSuchElementException nsee) {
- // Object was borrowed in another thread
- // Don't count this as an eviction test so reduce i;
- i--;
- evictionIterator = null;
- continue;
- }
-
- if (!underTest.startEvictionTest()) {
- // Object was borrowed in another thread
- // Don't count this as an eviction test so reduce i;
- i--;
- continue;
- }
-
- if (idleEvictTime < underTest.getIdleTimeMillis() ||
- (idleSoftEvictTime < underTest.getIdleTimeMillis() &&
- getMinIdle() < idleObjects.size())) {
- destroy(underTest);
- destroyedByEvictorCount.incrementAndGet();
- } else {
- if (testWhileIdle) {
- boolean active = false;
- try {
- factory.activateObject(underTest.getObject());
- active = true;
- } catch (Exception e) {
- destroy(underTest);
- destroyedByEvictorCount.incrementAndGet();
+ synchronized (evictionLock) {
+ boolean testWhileIdle = getTestWhileIdle();
+ long idleEvictTime = Long.MAX_VALUE;
+ long idleSoftEvictTime = Long.MAX_VALUE;
+
+ if (getMinEvictableIdleTimeMillis() > 0) {
+ idleEvictTime = getMinEvictableIdleTimeMillis();
+ }
+ if (getSoftMinEvictableIdleTimeMillis() > 0) {
+ idleSoftEvictTime = getSoftMinEvictableIdleTimeMillis();
+ }
+
+ for (int i = 0, m = getNumTests(); i < m; i++) {
+ if (evictionIterator == null || !evictionIterator.hasNext()) {
+ if (getLifo()) {
+ evictionIterator = idleObjects.descendingIterator();
+ } else {
+ evictionIterator = idleObjects.iterator();
}
- if (active) {
- if (!factory.validateObject(underTest.getObject())) {
+ }
+ if (!evictionIterator.hasNext()) {
+ // Pool exhausted, nothing to do here
+ return;
+ }
+
+ try {
+ underTest = evictionIterator.next();
+ } catch (NoSuchElementException nsee) {
+ // Object was borrowed in another thread
+ // Don't count this as an eviction test so reduce i;
+ i--;
+ evictionIterator = null;
+ continue;
+ }
+
+ if (!underTest.startEvictionTest()) {
+ // Object was borrowed in another thread
+ // Don't count this as an eviction test so reduce i;
+ i--;
+ continue;
+ }
+
+ if (idleEvictTime < underTest.getIdleTimeMillis() ||
+ (idleSoftEvictTime < underTest.getIdleTimeMillis() &&
+ getMinIdle() < idleObjects.size())) {
+ destroy(underTest);
+ destroyedByEvictorCount.incrementAndGet();
+ } else {
+ if (testWhileIdle) {
+ boolean active = false;
+ try {
+ factory.activateObject(underTest.getObject());
+ active = true;
+ } catch (Exception e) {
destroy(underTest);
destroyedByEvictorCount.incrementAndGet();
- } else {
- try {
- factory.passivateObject(underTest.getObject());
- } catch (Exception e) {
+ }
+ if (active) {
+ if
(!factory.validateObject(underTest.getObject())) {
destroy(underTest);
destroyedByEvictorCount.incrementAndGet();
+ } else {
+ try {
+
factory.passivateObject(underTest.getObject());
+ } catch (Exception e) {
+ destroy(underTest);
+ destroyedByEvictorCount.incrementAndGet();
+ }
}
}
}
- }
- if (!underTest.endEvictionTest(idleObjects)) {
- // TODO - May need to add code here once additional states
- // are used
+ if (!underTest.endEvictionTest(idleObjects)) {
+ // TODO - May need to add code here once additional
states
+ // are used
+ }
}
}
}
-
- return;
}
private PooledObject<T> create() throws Exception {
@@ -1223,14 +1223,16 @@ public class GenericObjectPool<T> extend
* milliseconds between evictor runs.
*/
// Needs to be final; see POOL-195. Make protected method final as it is
called from constructor.
- protected final synchronized void startEvictor(long delay) {
- if (null != evictor) {
- EvictionTimer.cancel(evictor);
- evictor = null;
- }
- if (delay > 0) {
- evictor = new Evictor();
- EvictionTimer.schedule(evictor, delay, delay);
+ protected final void startEvictor(long delay) {
+ synchronized (evictionLock) {
+ if (null != evictor) {
+ EvictionTimer.cancel(evictor);
+ evictor = null;
+ }
+ if (delay > 0) {
+ evictor = new Evictor();
+ EvictionTimer.schedule(evictor, delay, delay);
+ }
}
}
@@ -1522,11 +1524,6 @@ public class GenericObjectPool<T> extend
final private PoolableObjectFactory<T> factory;
/**
- * My idle object eviction {@link TimerTask}, if any.
- */
- private Evictor evictor = null; // @GuardedBy("this")
-
- /**
* All of the objects currently associated with this pool in any state. It
* excludes objects that have been destroyed. The size of
* {@link #allObjects} will always be less than or equal to {@link
@@ -1548,9 +1545,17 @@ public class GenericObjectPool<T> extend
private final LinkedBlockingDeque<PooledObject<T>> idleObjects =
new LinkedBlockingDeque<PooledObject<T>>();
+ /**
+ * My idle object eviction {@link TimerTask}, if any.
+ */
+ private Evictor evictor = null; // @GuardedBy("evictionLock")
+
/** An iterator for {@link #idleObjects} that is used by the evictor. */
- private Iterator<PooledObject<T>> evictionIterator = null;
+ private Iterator<PooledObject<T>> evictionIterator = null; //
@GuardedBy("evictionLock")
+ /** Object used to ensure thread safety of eviction process */
+ private final Object evictionLock = new Object();
+
/** Object used to ensure closed() is only called once. */
private final Object closeLock = new Object();
