Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Sandy McArthur]

On 3/28/06, Craig McClanahan <[EMAIL PROTECTED]> wrote:
> On 3/28/06, Peter Steijn <[EMAIL PROTECTED]> wrote:
> >
> > >
> > > I haven't dug into Dbcp code but I think the GenericObjectPool is deep
> > > within Dbcp as an internal data structure and I'd be surprised if it
> > > let you specify a specific pool implementation.
> >
> >
> > Does anyone else think that this should be different?
> >
> > Suggestion: The next version of DBCP should allow you to specify your own
> > implementation in a configuration file.  I know that I was looking around
> > for this feature when I first was exploring DBCP and POOL.
>
>
> It's been a couple of years since I looked in detail, but IIRC the only
> place that DBCP locks down the choice of the pool implementation is when you
> select a specific *implementation* of DataSource (such as
> org.apache.commons.dbcp.BasicDataSource).  If you assemble your own from
> scratch, you're free to do whatever you want as you build up the stack.
>
> Adding plugability inside something like BasicDataSource strikes me as an
> excellent way to hand a developer a loaded gun, aimed at their foot.  The
> code in a given implementation is going to naturally make assumptions about
> the underlying pool implementation being used (such as what configuration
> parameters it accepts) -- if you want a different DBCP implementation, it's
> really easy to assemble your own.

Also, my read of some of Dbcp code weeks ago is that if you use a pool
with a running idle object (connection) evictor you run the risk of
deadlock because of the order of how locks are acquired.

synchronization of getting a new connection normally:
DbcpConnection -> ObjectPool -> DbcpConnection.makeObject (implements
PoolableObjectFactory)
the same object instance on each side of the object pool instance.

The idle eviction thread synchronization:
ObjectPool -> DbcpConnection.activateObject (implements PoolableObjectFactory)

Until this is worked out it's not safe to let client code configure
their own ObjectPool. Most likely this problem won't turn up in
testing under light load but will fall apart in production.

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

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Craig McClanahan]

On 3/28/06, Peter Steijn <[EMAIL PROTECTED]> wrote:
>
> >
> > I haven't dug into Dbcp code but I think the GenericObjectPool is deep
> > within Dbcp as an internal data structure and I'd be surprised if it
> > let you specify a specific pool implementation.
>
>
> Does anyone else think that this should be different?
>
> Suggestion: The next version of DBCP should allow you to specify your own
> implementation in a configuration file.  I know that I was looking around
> for this feature when I first was exploring DBCP and POOL.


It's been a couple of years since I looked in detail, but IIRC the only
place that DBCP locks down the choice of the pool implementation is when you
select a specific *implementation* of DataSource (such as
org.apache.commons.dbcp.BasicDataSource).  If you assemble your own from
scratch, you're free to do whatever you want as you build up the stack.

Adding plugability inside something like BasicDataSource strikes me as an
excellent way to hand a developer a loaded gun, aimed at their foot.  The
code in a given implementation is going to naturally make assumptions about
the underlying pool implementation being used (such as what configuration
parameters it accepts) -- if you want a different DBCP implementation, it's
really easy to assemble your own.

-Pete
>
>
Craig

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Peter Steijn]

>
> I haven't dug into Dbcp code but I think the GenericObjectPool is deep
> within Dbcp as an internal data structure and I'd be surprised if it
> let you specify a specific pool implementation.


Does anyone else think that this should be different?

Suggestion: The next version of DBCP should allow you to specify your own
implementation in a configuration file.  I know that I was looking around
for this feature when I first was exploring DBCP and POOL.

-Pete

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Sandy McArthur]

On 3/28/06, Phil Steitz <[EMAIL PROTECTED]> wrote:
> On 3/28/06, robert burrell donkin <[EMAIL PROTECTED]> wrote:
> > On Mon, 2006-03-27 at 19:43 -0700, Phil Steitz wrote:
> > > 
> > > > * 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
> > > case.
> >
> > or replace with a mock pool implementation (if possible)
>
> This is an interesting idea.  Would appreciate suggestions on how
> exactly to do this.

I haven't dug into Dbcp code but I think the GenericObjectPool is deep
within Dbcp as an internal data structure and I'd be surprised if it
let you specify a specific pool implementation.

Even if you do mock the pool, the test would be testing the behavior
of the mock pool as called through Dbcp. I think any interesting
interaction with the GOP would be implicitly tested by other unit
tests.

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

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Phil Steitz]

On 3/28/06, robert burrell donkin <[EMAIL PROTECTED]> wrote:
> On Mon, 2006-03-27 at 19:43 -0700, Phil Steitz wrote:
> > 
> > > * 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
> > case.
>
> or replace with a mock pool implementation (if possible)

This is an interesting idea.  Would appreciate suggestions on how
exactly to do this.

Phil

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

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[robert burrell donkin]

On Mon, 2006-03-27 at 19:43 -0700, Phil Steitz wrote:
> 
> > * 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
> case.

or replace with a mock pool implementation (if possible)

- robert


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

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Phil Steitz]

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

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

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

Phil

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

Re: Dbcp unit test failures with Pool 1.3 [was: [VOTE] Release Pool 1.3 based on 1.3-rc4]

[Sandy McArthur]

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.

> 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