commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phil Steitz" <>
Subject Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]
Date Tue, 28 Mar 2006 02:43:21 GMT
> 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.

Yes.  This is bad and I agree this test case should be removed, as it
depends on the implementation of the underlying pool, which is not
part of [dbcp].  If there are no objections, I will remove this test

> * 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.

As you point out below, this is definitely true of testPooling when it
fails (as it does) with the new pool impl.

> * 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.

Yes.  Better cleanup needs to be included in these tests.

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

Ack.  Patches welcome!

Based on above, I am +1 for pool release.


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message