Mark Thomas wrote:
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.
I think you are right about _poolMap and _poolList, but what about the
actual object queues? Not clearing them after destroying the objects
will leave the object cursors uncleared. Could be I am blind, but I do
not see the code clearing the queues. I am now confused as to why the
change I committed
appeared to fix the problem with testEvictorVisiting. See below. Sorry
again for the top-posting munging order here.
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.
I don't think so. The test code is *validating* idle objects, not
destroying them.
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.
That part I agree with - the clearing cursors resolves the problem
part. But unfortunately (as you point out in another message), the
cursors should pick up the list modifications, so this does not quite
explain things.
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.
This is a separate issue (thanks!) that I don't think is operative
here, but I agree strongly with the suggestion to fix (unfortunately not
until 2.0).
I will try to write some tests to get to the bottom of the
testEvictorVisiting test.
Phil
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
