Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: neutral (nike.apache.org: local policy)
Message-ID: <4A25836C.9070302@apache.org>
Date: Tue, 02 Jun 2009 20:54:20 +0100
From: Mark Thomas <markt@apache.org>
User-Agent: Thunderbird 2.0.0.21 (Windows/20090302)
MIME-Version: 1.0
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: svn commit: r780905 -
 /commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
References: <20090602020123.2EEBA23888BD@eris.apache.org>
	 <4A251578.6060007@apache.org>
	 <8a81b4af0906020535x503fe79av86ba113138b519cc@mail.gmail.com>
	 <4A255BB0.8070805@apache.org>
 <8a81b4af0906021249o1e1c9144g22393599fd6e7b80@mail.gmail.com>
In-Reply-To: <8a81b4af0906021249o1e1c9144g22393599fd6e7b80@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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.

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


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