commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: svn commit: r780905 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
Date Wed, 03 Jun 2009 00:13:34 GMT
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.
>>>  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.
>>> I have tried (and failed) to add
>>> some tests for this. What I really want to do have the factory destroy()
>>> method throw an exception if you try to destroy the same object twice.
>>> Unfortunately, the pool implementations just swallow this exception.
>>>       
This is a separate issue (thanks!)  that I don't think is operative 
here, but I agree strongly with the suggestion to fix (unfortunately not 
until 2.0).

 I will try to write some tests to get to the bottom of the 
testEvictorVisiting test.

Phil
>>> Logging (ie POOL 2.x) could be a solution to this. Another option is to
>>> remove the throws declaration from the factory destroy() method and
>>> leave it up to factory implementations to determine what exceptions are
>>> safe to be swallowed. That is a small, but very significant, API change
>>> that needs discussion come 2.x
>>>
>>> I'll revert r780905, add the fix described above and than re-review the
>>> dev archive and look at the other potential issues identified over the
>>> last few days.
>>>
>>> Mark
>>>
>>>       
>>>> On 6/2/09, Mark Thomas <markt@apache.org> wrote:
>>>>         
>>>>> psteitz@apache.org wrote:
>>>>>           
>>>>>> Author: psteitz
>>>>>> Date: Tue Jun  2 02:01:22 2009
>>>>>> New Revision: 780905
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=780905&view=rev
>>>>>> Log:
>>>>>> Ensure that clear() fully clears the pool.
>>>>>>             
>>>>> <snip/>
>>>>>
>>>>>           
>>>>>> @@ -1293,6 +1293,7 @@
>>>>>>              }
>>>>>>          }
>>>>>>          destroy(toDestroy);
>>>>>> +        _poolMap.clear();
>>>>>>      }
>>>>>>
>>>>>>      /**
>>>>>>             
>>>>> Still working through the e-mails from over the weekend but on first
>>>>> inspection that _poolMap.clear() needs to be inside the sync block to
>>>>> keep it thread safe. Given that, I am having troubling seeing how the
>>>>> _poolMap isn't already empty at that point. I wonder if we got to the
>>>>> real root cause of the bug ...?
>>>>>
>>>>> I'll get set up to do some testing and report my findings.
>>>>>
>>>>> Mark
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>>>
>>>>>           
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>         
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>>       
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>     
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>   


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


Mime
View raw message