You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Wayne Woodfield <ja...@woodfieldfamily.org> on 2004/04/19 07:04:41 UTC
PATCH [dbcp] fix for negative numActive
I'm absolutely new to open-source development and groups like this, so
forgive me if I'm not following the correct protocol, but I found and
fixed a bug in DBCP and would like to contribute my patch back to the
project. I've created a patch file and attached it to this e-mail. If
a comitter to commons-dbcp would please look at my patch as well as the
explanation below, that'd be great. If you'd like me to do anything
differently next time I have a patch to offer, just let me know :-)
Wayne Woodfield
-------------------------
BUG DESCRIPTION:
I discovered this bug while using Tomcat DBCP. I was using
removeAbandoned=true, so AbandonedObjectPool was used. We had
maxActive=10 and maxIdle=2. Under a really heavy load, numActive gets
off. Over time, well over a hundred connections to the database get
spawned at the same time. Because numActive gets so far off, it seems
like the maxActive limit is ignored. The database runs out of
connections to give. After the heavy load has passed and things go back
to normal, a call to DataSource.getNumActive() shows that numActive is
negative.
CAUSE:
I was able to trace this problem back to a bug in AbandonedObjectPool.
AbandonedObjectPool initiates an abandoned connection check every time
someone tries to borrow a connection from an exhausted or
nearly-exhausted connection pool. That means that several threads could
initiate an abandoned connection sweep at once. If two abandoned
connection checks running at the same time try to invalidate the same
connection, they will both be allowed to do so. GenericObjectPool
(where numActive is stored) doesn't have any way of knowing whether a
connection has been invalidated before. It just decrements numActive
again. A connection can also be invalidated and returned to the idle
pool at the same time, again decrementing numActive twice.
AbandonedObjectPool keeps track of active connections. When a
connection is returned or invalidated, the connection is removed from
the active list. AbandonedObjectPool should check to see whether the
connection is still in the active list before removing it, and it
doesn't.
SOLUTION:
In AbandonedObjectPool's returnObject(Object obj) and
invalidateObject(Object obj) methods, I've added a few lines to ensure
that connections cannot be invalidated or returned twice. Before
invalidating a connection or returning a connection to the idle pool,
I'm simply checking to see whether the connection to return or
invalidate is still in the active connection list. If it isn't, then
this connection has already been invalidated, and the return or
invalidation is aborted.
I took advantage of the fact that calling remove(Object) on an ArrayList
returns true if the object to remove was actually found, and false
otherwise, so I didn't even have to do an additional traversal of the
active connection list to find out whether the connection had already
been removed, so there is practically no performance impact at all :-)
See the attached patch.
Re: PATCH [dbcp] fix for negative numActive
Posted by Wayne Woodfield <ja...@woodfieldfamily.org>.
Thanks Dirk! I've worked up a good JUnit test for this bug & fix, and
I'm just about finished with it. I'll probably have time to put the
finishing touches on it within the next few days.
Wayne Woodfield
Dirk Verbeeck wrote:
> patch applied
>
> -- Dirk
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: PATCH [dbcp] fix for negative numActive
Posted by Dirk Verbeeck <di...@pandora.be>.
patch applied
-- Dirk
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [dbcp][bug 28579] JUnit test
Posted by Dirk Verbeeck <di...@pandora.be>.
applied...
-- Dirk
Wayne Woodfield wrote:
> When I wrote this JUnit test, my fake PoolableObjectFactory simulated destroying an
> object by waiting 30ms. I did this to ensure that the threads waited right there,
> allowing other threads to run and cause errors. Well, it turns out that I didn't
> really need to wait for any time at all. I can get the same testing effect by just
> calling Thread.yield() instead of sleeping. And it makes the test run much faster.
>
> Also, since spaces are preferred, I removed a few lingering tabs as well. The
> attached patch implements these two things.
>
> Thanks!
> Wayne
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [dbcp][bug 28579] JUnit test
Posted by Wayne Woodfield <ja...@woodfieldfamily.org>.
When I wrote this JUnit test, my fake PoolableObjectFactory simulated destroying an
object by waiting 30ms. I did this to ensure that the threads waited right there,
allowing other threads to run and cause errors. Well, it turns out that I didn't
really need to wait for any time at all. I can get the same testing effect by just
calling Thread.yield() instead of sleeping. And it makes the test run much faster.
Also, since spaces are preferred, I removed a few lingering tabs as well. The
attached patch implements these two things.
Thanks!
Wayne
Dirk Verbeeck wrote:
> JUnit test committed.
>
> Using spaces is preferred:
> http://jakarta.apache.org/commons/patches.html
Re: [dbcp][bug 28579] JUnit test
Posted by Dirk Verbeeck <di...@pandora.be>.
JUnit test committed.
Using spaces is preferred:
http://jakarta.apache.org/commons/patches.html
Cheers
Dirk
Wayne Woodfield wrote:
> This JUnit test will test the fix for bug 28579. It takes between 1-2
> seconds to complete on my computer. Even though it simulates a threading
> issue, it's very reliable. I haven't been able to get it to generate a false
> positive yet. If you commit this new test to CVS, please insert a line for
> this test into the org.apache.commons.dbcp.TestAll class. I haven't done so
> yet.
>
> By the way, I often use tab characters for indentation, and so I have done so
> with this file. If it is the jakarta or commons standard to use spaces for
> indentation, I'll use that convention from now on.
>
> Look it over -- if any questions or comments, let me know.
>
> Wayne Woodfield
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
[dbcp][bug 28579] JUnit test
Posted by Wayne Woodfield <ja...@woodfieldfamily.org>.
This JUnit test will test the fix for bug 28579. It takes between 1-2
seconds to complete on my computer. Even though it simulates a threading
issue, it's very reliable. I haven't been able to get it to generate a false
positive yet. If you commit this new test to CVS, please insert a line for
this test into the org.apache.commons.dbcp.TestAll class. I haven't done so
yet.
By the way, I often use tab characters for indentation, and so I have done so
with this file. If it is the jakarta or commons standard to use spaces for
indentation, I'll use that convention from now on.
Look it over -- if any questions or comments, let me know.
Wayne Woodfield
Dirk Verbeeck wrote:
> Hi Wayne,
>
> Excellent description and patch. The commons-dev mailing list is
> indeed the place to start for getting your fix applied. Now a bugzilla
> issue needs to be created to record the issue+patch and then I will
> apply your patch to DBCP.
> Do you have a way to simulate the error? Threading issues are always
> difficult to reproduce and your solution is obviously correct but
> having a JUnit test is always a plus.
>
> Thanks
> Dirk
>
> Wayne Woodfield wrote:
> > I'm absolutely new to open-source development and groups like this, so
> > forgive me if I'm not following the correct protocol, but I found and
> > fixed a bug in DBCP and would like to contribute my patch back to the
> > project. I've created a patch file and attached it to this e-mail. If
> > a comitter to commons-dbcp would please look at my patch as well as the
> > explanation below, that'd be great. If you'd like me to do anything
> > differently next time I have a patch to offer, just let me know :-)
> >
> > Wayne Woodfield
> > -------------------------
Re: PATCH [dbcp] fix for negative numActive
Posted by Dirk Verbeeck <di...@pandora.be>.
Hi Wayne,
Excellent description and patch. The commons-dev mailing list is
indeed the place to start for getting your fix applied. Now a bugzilla
issue needs to be created to record the issue+patch and then I will
apply your patch to DBCP.
Do you have a way to simulate the error? Threading issues are always
difficult to reproduce and your solution is obviously correct but
having a JUnit test is always a plus.
Thanks
Dirk
Wayne Woodfield wrote:
> I'm absolutely new to open-source development and groups like this, so
> forgive me if I'm not following the correct protocol, but I found and
> fixed a bug in DBCP and would like to contribute my patch back to the
> project. I've created a patch file and attached it to this e-mail. If
> a comitter to commons-dbcp would please look at my patch as well as the
> explanation below, that'd be great. If you'd like me to do anything
> differently next time I have a patch to offer, just let me know :-)
>
> Wayne Woodfield
> -------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org