Anyone up for a game of Clue?

I think testThreading did it, with assertTrue in TestConnectionPool.

On 3/26/06, Phil Steitz <[EMAIL PROTECTED]> wrote:
> -0
> Could be a problem with setup or test itself, but with the 1.3-rc4
> jar, I am getting failures in [dbcp] test
> org.apache.commons.dbcp.TestJOCLed.  These tests pass with 1.2.  The
> good news is the other [dbcp] test failures that pool 1.3 is supposed
> to fix succeed for me.  Failures below happen on both Sun Linux JDK
> 1.5.0_06 and 1.4.2_10.
<snip/>
> Checked sigs and contents and everything looks good.  Could be test
> failure is due to bad test in [dbcp].  Will change to +1 when this is
> resolved.
>
> Phil
>
> Test failure:
>
> Testcase: testPooling(org.apache.commons.dbcp.TestJOCLed):      FAILED
> null
> junit.framework.AssertionFailedError
>         at 
> org.apache.commons.dbcp.TestConnectionPool.testPooling(TestConnectionPool.java:270)
>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>         at 
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
>         at 
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
>
>
> Testcase: testConnectionsAreDistinct(org.apache.commons.dbcp.TestJOCLed):     
>   Caused
> an ERROR
> Cannot get a connection, pool error: Timeout waiting for idle object
> org.apache.commons.dbcp.SQLNestedException: Cannot get a connection,
> pool error: Timeout waiting for idle object
>         at 
> org.apache.commons.dbcp.PoolingDriver.connect(PoolingDriver.java:183)
>         at java.sql.DriverManager.getConnection(DriverManager.java:525)
>         at java.sql.DriverManager.getConnection(DriverManager.java:193)
>         at 
> org.apache.commons.dbcp.TestJOCLed.getConnection(TestJOCLed.java:42)
>         at 
> org.apache.commons.dbcp.TestConnectionPool.testConnectionsAreDistinct(TestConnectionPool.java:296)
>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>         at 
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
>         at 
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
> Caused by: java.util.NoSuchElementException: Timeout waiting for idle object
>         at 
> org.apache.commons.pool.impl.GenericObjectPool.borrowObject(GenericObjectPool.java:825)
>         at 
> org.apache.commons.dbcp.PoolingDriver.connect(PoolingDriver.java:175)
>         ... 18 more
>
> This same error occurs for testOpening, testClosing,  testMaxActive,
> testThreaded, testPrepareStatementOptions, testNoRsetClose and
> testHashCode.

Who did it? How did it happen? Lets look at the clues.

First while we are running TestJOCLed, it subclasses
TestConnectionPool and that is where the failures are really
happening.

* testPooling: This test method passes when you run it by itself. It
fails because while it looks like it's written to handle either a FIFO
or a LIFO pool implementation it doesn't if the GenericObjectPool
backing Dbcp has more than 2 idle connections. When the test is run
after other tests the GOP contains 4 idle connections. With a LIFO the
most recently returned is the first one borrowed so it doesn't matter
how many idle connections already exist at the start of the test.
Since GOP is now a FIFO the test fails because is incorrectly assumes
that a recently returned connection will be the next one borrowed. IMO
testPooling should be removed as it really testing Pool's behavior and
not Dbcp's behavior.

* testConnectionsAreDistinct: This test method passes when you run it
by itself. Took me a while to figure out why this fails. It fails
because the GenericObjectPool backing Dbcp is configured with a
maxActive of 10 but for me the test fails after borrowing 9 successful
connections. I'm pretty sure somewhere two of the test methods are
borrowing connections and not following the proper contracts and
closing their connections.

* testOpening, testClosing, testMaxActive, testThreaded,
testPrepareStatemetnOptions, testNoRsetClose, testHashCode: all pass
when run individually. They all fail because when
testConnectionsAreDistinct fails it doesn't close any connection's it
opened in a finally block so the shared GenericObjectPool which has a
maxActive set to 10 thinks it's maxed out with 10 borrowed connections
(9 were from testConnectionsAreDistinct).

This whole unit test class has a problem in that each test method is
not independent of the others. Each test method works when run alone
because they have a fresh state that way. It's when one test leaves
garbage state around after it runs that is affects other tests and
triggers a cascade of failures.

So where is the garbage state coming from? Like I said at the top,
testPooling did it with the assertTrue, specifically lines 270 and 271
of TestConnectionPool:

270: assertTrue( underconn3 == underconn || underconn3 == underconn2 );
271: conn3.close();

When the assertTrue throws an exception the conn3.close is never
called resulting in a connection leak.

The easiest fix is to just transpose those two lines and everything
else but testPooling passes just fine. That still leaves testPooling
as failing and I think that should be solved by removing the test
method completely. I think it's fair to say that unit tests for Pool
belong in the Pool component code base.

Still, someone should look at the testConnectionsAreDistinct test
method as it doesn't clean up after itself when it fails.

Also someone should rework the unit test as a whole so when each test
method is run there is no residual state left over from other test
methods. Until this is fixed TestConnectionPool isn't really testing
what you think it is and probably technically broken.

--
Sandy McArthur

"He who dares not offend cannot be honest."
- Thomas Paine

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to