Author: markt
Date: Tue May 26 20:33:57 2009
New Revision: 778875
URL: http://svn.apache.org/viewvc?rev=778875&view=rev
Log:
Fix two issues that, oddly, had the same fix.
1. clearOldest() was called from inside a sync block. clearOldest() uses
factory methods so must not be called from inside a sync block to prevent
possible deadlocks.
2. clearOldest() calls allocate which set up a recursive call to allocate()
which in turn emptied the allocation queue. This caused breakage as the
recursive call unwound and items expected to be in the allocation queue were no
longer there.
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
URL:
http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java?rev=778875&r1=778874&r2=778875&view=diff
==============================================================================
---
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
(original)
+++
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
Tue May 26 20:33:57 2009
@@ -1206,56 +1206,72 @@
* set _mayCreate to true for as many additional latches remaining in queue
* as _maxActive allows for each key.
*/
- private synchronized void allocate() {
- if (isClosed()) return;
+ private void allocate() {
+ boolean clearOldest = false;
- for (;;) {
- if (!_allocationQueue.isEmpty()) {
- // First use any objects in the pool to clear the queue
- Latch latch = (Latch) _allocationQueue.getFirst();
- ObjectQueue pool = (ObjectQueue)(_poolMap.get(latch.getkey()));
- if (null == pool) {
- pool = new ObjectQueue();
- _poolMap.put(latch.getkey(), pool);
- _poolList.add(latch.getkey());
- }
- latch.setPool(pool);
- if (!pool.queue.isEmpty()) {
- _allocationQueue.removeFirst();
- latch.setPair(
- (ObjectTimestampPair) pool.queue.removeFirst());
- pool.incrementInternalProcessingCount();
- _totalIdle--;
- synchronized (latch) {
- latch.notify();
+ synchronized (this) {
+ if (isClosed()) return;
+
+ for (;;) {
+ if (!_allocationQueue.isEmpty()) {
+ // First use any objects in the pool to clear the queue
+ Latch latch = (Latch) _allocationQueue.getFirst();
+ ObjectQueue pool =
(ObjectQueue)(_poolMap.get(latch.getkey()));
+ if (null == pool) {
+ pool = new ObjectQueue();
+ _poolMap.put(latch.getkey(), pool);
+ _poolList.add(latch.getkey());
}
- // Next item in queue
- continue;
- }
-
- // If there is a totalMaxActive and we are at the limit then
- // we have to make room
- if ((_maxTotal > 0) &&
- (_totalActive + _totalIdle + _totalInternalProcessing
>= _maxTotal)) {
- clearOldest();
- }
-
-
- // Second utilise any spare capacity to create new objects
- if ((_maxActive < 0 || pool.activeCount +
pool.internalProcessingCount < _maxActive) &&
- (_maxTotal < 0 || _totalActive + _totalIdle +
_totalInternalProcessing < _maxTotal)) {
- // allow new object to be created
- _allocationQueue.removeFirst();
- latch.setMayCreate(true);
- pool.incrementInternalProcessingCount();
- synchronized (latch) {
- latch.notify();
+ latch.setPool(pool);
+ if (!pool.queue.isEmpty()) {
+ _allocationQueue.removeFirst();
+ latch.setPair(
+ (ObjectTimestampPair)
pool.queue.removeFirst());
+ pool.incrementInternalProcessingCount();
+ _totalIdle--;
+ synchronized (latch) {
+ latch.notify();
+ }
+ // Next item in queue
+ continue;
+ }
+
+ // If there is a totalMaxActive and we are at the limit
then
+ // we have to make room
+ if ((_maxTotal > 0) &&
+ (_totalActive + _totalIdle +
_totalInternalProcessing >= _maxTotal)) {
+ clearOldest = true;
+ break;
+ }
+
+ // Second utilise any spare capacity to create new objects
+ if ((_maxActive < 0 || pool.activeCount +
pool.internalProcessingCount < _maxActive) &&
+ (_maxTotal < 0 || _totalActive + _totalIdle +
_totalInternalProcessing < _maxTotal)) {
+ // allow new object to be created
+ _allocationQueue.removeFirst();
+ latch.setMayCreate(true);
+ pool.incrementInternalProcessingCount();
+ synchronized (latch) {
+ latch.notify();
+ }
+ // Next item in queue
+ continue;
}
- // Next item in queue
- continue;
}
+ break;
}
- break;
+ }
+
+ if (clearOldest) {
+ /* Clear oldest calls factory methods so it must be called from
+ * outside the sync block.
+ * It also needs to be outside the sync block as it calls
+ * allocate(). If called inside the sync block, the call to
+ * allocate() would be able to enter the sync block (since the
+ * thread already has the lock) which may have unexpected,
+ * unpleasant results.
+ */
+ clearOldest();
}
}
