commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r780905 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
Date Wed, 03 Jun 2009 06:24:57 GMT
Phil Steitz wrote:
> Mark Thomas wrote:
>> Phil Steitz wrote:
>>  
>>> +1 thanks.
>>>
>>> Are you sure not clearing the queues, map and list on clear is OK?
>>> Could this result in memory leaks in some apps?
>>>     
>>
>> I am 99.9% sure that the queues map and list will all be clear at the
>> end of the clear() method. At least, that was my intention. I'll
>> re-check the code. If that isn't the case, I'd rather figure out where
>> the new code is wrong than add a band-aid in case it ends up masking
>> some other subtle issue. We might need to add some additional checks
>> just while we complete the testing.
>>   
> I think you are right about _poolMap and _poolList, but what about the
> actual object queues?  Not clearing them after destroying the objects
> will leave the object cursors uncleared.  Could be I am blind, but I do
> not see the code clearing the queues.   I am now confused as to why the
> change I committed
> appeared to fix the problem with testEvictorVisiting.  See below.  Sorry
> again for the top-posting munging order here.
>> Mark
>>
>>  
>>> On 6/2/09, Mark Thomas <markt@apache.org> wrote:
>>>    
>>>> Phil Steitz wrote:
>>>>      
>>>>> Good catch. Yes, needs to by synched (and that could be tricky, with
>>>>> destroy between). Looks to me like destroy destroys the pairs, but
>>>>> does not clear the queues (prior to change) and clear clears _poolList
>>>>> but not _poolMap. Could be I am wrong. I Will look at this some more
>>>>> this eve.
>>>>>         
>>>> I did some more digging and I think r780905 needs to be reverted.
>>>>
>>>> The root cause is an issue you and Sebb had already identified - the
>>>> eviction cursors are not reset on clear(). Stepping through the code
>>>> what happens is that the first eviction attempts to destroy an object
>>>> that has already been destroyed.
> I don't think so.  The test code is *validating* idle objects, not
> destroying them.

My mistake. Le me try again. The first eviction attempts attempts to
validate an object that has already been destroyed (from the previous run).

I need to do some more testing to figure out why an object is no longer
in the pool but is in the cursor.

>>>>  This appears to succeed - hence why we
>>>> end up one eviction short at the end of the test.
>>>>
>>>> I can reproduce the problem and clearing the eviction cursors as
>>>> part fo
>>>> clear appears to resolve the issue. 
> That part I agree with - the clearing cursors resolves the problem
> part.  But unfortunately (as you point out in another message), the
> cursors should pick up the list modifications, so this does not quite
> explain things.

Having slept on the problem, I agree with you. Neither, r780905 nor
r781166 should be required. Something else is going wrong. I'm going to
revert r781166 and do some more testing.

Mark


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


Mime
View raw message