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 17:04:48 GMT
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.


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

View raw message