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 <ma...@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 <ma...@apache.org> wrote: >>>> pste...@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-unsubscr...@commons.apache.org >>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>> >>>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
