Mailing-List: contact commons-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Message-ID: <33339729.1162338739316.JavaMail.root@brutus>
Date: Tue, 31 Oct 2006 15:52:19 -0800 (PST)
From: "Sandy McArthur (JIRA)" <jira@apache.org>
To: commons-dev@jakarta.apache.org
Subject: [jira] Commented: (POOL-86) GenericKeyedObjectPool retaining too
 many idle objects
In-Reply-To: <2268038.1160521399565.JavaMail.root@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

    [ 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