[prev in list] [next in list] [prev in thread] [next in thread] 

List:       jakarta-commons-dev
Subject:    [jira] Commented: (POOL-86) GenericKeyedObjectPool retaining too
From:       "Sandy McArthur (JIRA)" <jira () apache ! org>
Date:       2006-10-31 23:52:19
Message-ID: 33339729.1162338739316.JavaMail.root () brutus
[Download RAW message or body]

    [ http://issues.apache.org/jira/browse/POOL-86?page=comments#action_12446123 ] 
            
Sandy McArthur commented on POOL-86:
------------------------------------

> I think what I want, and what most DB connection pools need, is the existing
> idle eviction facility as advertised.

I can appreciate that you probably think I'm just being bone headed. But idle object \
eviction as implemented by pool runs the risk of deadlocks and it cannot be fixed \
without breaking backwards compatibility. When considering only one usage of pool it \
is possible to use the evictor in a thread-safe manner but it would be irresponsible \
of me to encourage use of a risky pool feature when the desired results can be \
implemented in a manner that is thread-safe in every use case.

I'm trying to provide a solution that will meet your needs and be deadlock proof for \
everyone else at the same time. If you don't want to work with me on this then you \
are free to maintain your own version of a pool under the terms of the ASL-2.0.


> I've never encountered the thread-safety problems you mention. Is there an
> outstanding bug regarding that?

DBCP-65 is one example: https://issues.apache.org/jira/browse/DBCP-65

The problem is locks are acquired in reverse order. If there are two locks, \
ClientLock and PoolLock and the ClientLock is aquired by code using pool and by the \
PoolableObjectFactory provided to the pool you run the risk of deadlock when using an \
evitor.

Normal usage will acquire ClientLock, call a pool method which will acquire the \
internal PoolLock, and then the pool will call the PoolableObjectFactory which also \
acquires the ClientLock again. Effectively the lock acquistion order is ClientLock \
then PoolLock.

A pool with an evictor thread won't know about the ClientLock. After it wakes up to \
do idle object eviction it will acquire the PoolLock and the call the \
PoolableObjectFactory which will acquire the ClientLock. In the case the lock \
acquisition is PoolLock then ClientLock.

Acquiring locks in reverse order is the most basic example of how to cause a deadlock \
in any text on concurrency.


> If so, it's not occurring in my environment
> which as I said involves hundreds of keys (DB users), thousands of connections,
> and in fact is used by hundreds of threads.

I'm not privy to your code. I don't know if your code is thread-safe or just lucky. \
Just because pool works in a particular setup doesn't change the fact that idle \
object eviction is risky and should be avoided.


> _recentlyEvictedKeys can serve the same purpose. This change would do that: 

Almost, this has the same problem of possibly skipping objects eligible for eviction \
which is the same problem we have now mentioned in item #2 of your original post.

> GenericKeyedObjectPool retaining too many idle objects
> ------------------------------------------------------
> 
> Key: POOL-86
> URL: http://issues.apache.org/jira/browse/POOL-86
> Project: Commons Pool
> Issue Type: Bug
> Affects Versions: 1.3
> Reporter: Mike Martin
> Assigned To: Sandy McArthur
> Attachments: pool-86.patch, pool-86.withtest.patch
> 
> 
> There are two somewhat related problems in GenericKeyedObjectPool that cause
> many more idle objects to be retained than should be, for much longer than they
> should be.
> Firstly, borrowObject() is returning the LRU object rather than the MRU object.
> That minimizes rather than maximizes object reuse and tends to refresh all the
> idle objects, preventing them from becoming evictable.
> The idle LinkedList is being maintained with:
> borrowObject:   list.removeFirst()
> returnObject:   list.addLast()
> These should either both be ...First() or both ...Last() so the list maintains
> a newer-to-older, or vice-versa, ordering.  The code in evict() works from the
> end of the list which indicates newer-to-older might have been originally
> intended.
> Secondly, evict() itself has a couple of problems, both of which only show up
> when many keys are in play:
> 1.  Once it processes a key it doesn't advance to the next key.
> 2.  _evictLastIndex is not working as documented ("Position in the _pool where
> the _evictor last stopped").  Instead it's the position where the last scan
> started, and becomes the position at which it attempts to start scanning
> *in the next pool*.  That just causes objects eligible for eviction to
> sometimes be skipped entirely.
> Here's a patch fixing both problems:
> GenericKeyedObjectPool.java
> 990c990
> <             pool.addLast(new ObjectTimestampPair(obj));
> ---
> > pool.addFirst(new ObjectTimestampPair(obj));
> 1094,1102c1094,1095
> <                 }
> <
> <                 // if we don't have a keyed object pool iterator
> <                 if (objIter == null) {
> <                     final LinkedList list = (LinkedList)_poolMap.get(key);
> <                     if (_evictLastIndex < 0 || _evictLastIndex > list.size()) {
> <                         _evictLastIndex = list.size();
> <                     }
> <                     objIter = list.listIterator(_evictLastIndex);
> ---
> > LinkedList list = (LinkedList)_poolMap.get(key);
> > objIter = list.listIterator(list.size());
> 1154,1155c1147
> <                     _evictLastIndex = -1;
> <                     objIter = null;
> ---
> > key = null;
> 1547,1551d1538
> <
> <     /**
> <      * Position in the _pool where the _evictor last stopped.
> <      */
> <     private int _evictLastIndex = -1;
> I have a local unit test for this but it depends on some other code I can't
> donate.  It works like this:
> 1.  Fill the pool with _maxTotal objects using many different keys.
> 2.  Select X as a small number, e.g. 2.
> 3.  Compute:
> maxEvictionRunsNeeded = (maxTotal - X) / numTestsPerEvictionRun + 2;
> maxEvictionTime = minEvictableIdleTimeMillis + maxEvictionRunsNeeded * \
> timeBetweenEvictionRunsMillis; 4.  Enter a loop:
> a.  Borrow X objects.
> b.  Exit if _totalIdle = 0
> c.  Return the X objects.
> Fail if loop doesn't exit within maxEvictionTime.
> Mike

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: \
                http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


[prev in list] [next in list] [prev in thread] [next in thread]