commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <>
Subject Re: svn commit: r780905 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/
Date Tue, 02 Jun 2009 19:54:20 GMT
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.


> On 6/2/09, Mark Thomas <> 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 <> wrote:
>>>> wrote:
>>>>> Author: psteitz
>>>>> Date: Tue Jun  2 02:01:22 2009
>>>>> New Revision: 780905
>>>>> URL:
>>>>> 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:
>>>> For additional commands, e-mail:
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:
>>> For additional commands, e-mail:
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> For additional commands, e-mail:
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message