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 Tue, 02 Jun 2009 19:49:42 GMT
+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?

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. 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. 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.
>
> 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


Mime
View raw message