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