commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sandy McArthur (JIRA)" <j...@apache.org>
Subject [jira] Commented: (POOL-86) GenericKeyedObjectPool retaining too many idle objects
Date Tue, 31 Oct 2006 23:52:19 GMT
    [ 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


Mime
View raw message