[DBCP-269] final jdbc connection never closed --> number of connections grows

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 1.3, 1.4, Nightly Builds
• Fix Version/s: 1.3

Environment

Windows, JDK 5

• Assignee: Dain Sundstrom
• Reporter: Jean-Louis Monteiro
• Created: 12/Jun/08 09:23
• Updated: 16/Jun/08 20:26
• Resolved: 16/Jun/08 20:26

Description

Using DBCP parameters:

JdbcDriver com.mysql.jdbc.Driver
JdbcUrl jdbc:mysql://localhost:3307/dbcp?cacheResultsetMetadata=true
UserName dbcp
Password dbcp
InitialSize 5
MaxActive 8
MaxWait -1
TestOnBorrow false
TestOnReturn false
PoolPreparedStatements true
MaxOpenPreparedStatements 10

1. TimeBetweenEvictionRunsMillis 20000
2. MinEvictableIdleTimeMillis 30000
3. NumTestsPerEvictionRun 3
4. TestWhileIdle false
   MinIdle 2
   MaxIdle 5
   DefaultTransactionIsolation 1

provides strange behaviour.
The testcase is simple:
@Test
public void testMaxIdle() throws Exception {
int MAX_ACTIVE = 8;
List<Connection> connectionList = new ArrayList<Connection>();
int i = 0;
DataSource dataSource = ...; // retrieve the datasource
Assert.assertNotNull(dataSource);

System.out.println("*** Getting all connections ...");
for (i = 0; i < MAX_ACTIVE; i++)

{ System.out.println("get connection " + (i + 1)); Connection con = dataSource.getConnection(); Assert.assertNotNull(con); connectionList.add(con); }

i = 0;
System.out.println("*** Releasing all connections ...");
for (Iterator<Connection> iterator = connectionList.iterator(); iterator.hasNext() { Connection connection = (Connection) iterator.next(); System.out.println("close connection " + (++i)); connection.close(); iterator.remove(); }

i = 0;
System.out.println("*** Getting all + X connections ...");
for (i = 0; i < (MAX_ACTIVE + 1); i++) { System.out.println("get connection " + (i + 1)); Connection con = dataSource.getConnection(); Assert.assertNotNull(con); connectionList.add(con); }

}

First of all, after initialization, the number of real database connections established is 6 instead of 5. This due to the "protected synchronized DataSource createDataSource" method of the BasicDataSource object because of the validation of the ConnectionFactory. The "validateConnectionFactory" method creates a new jdbc connection and destroy it at the end. Nonetheless, the underlying database connection is not really closed.

At the end of the above test case, we should not have more than 8 connections but we have in fact 12 of them (8 + 1 for validation + 3 because DBCP wants to close extra connection MAX_ACTIVE - MAX_IDLE). In fact, those 3 connections are not really closed.

Please find attached a patch correcting this bug. This is due to the method isClosed() of the DelegatingConnection. From my point of view, the implementation of the method should be:

public boolean isClosed() throws SQLException {
if(_closed && _conn.isClosed())

{ // Instead of if(_closed || _conn.isClosed()) return true; }

return false;
}

From my understanding, a connection is considered as closed if and only if one of the connection chain is closed (OR in the test). But, if I'm write the _closed flag of the delegating connection is set to true during the passivation. So, after returning to the pool, a connection can not be closed. The isClosed method is really important because it's used indirectly by the reallyClose method of a PoolableConnection.

The behaviour is much more impressive if the evict thread of the configuration is activated because, with a testcase that simply wait after getting the datasource initialized (Thread.currentThread().sleep(600000) you will see database connections growing.

Finally, after applying the given patch, I faced some problems regarding DBCP TestCases:
In some test cases, you have a test method
public void testClose() throws Exception

{ ds.setAccessToUnderlyingConnectionAllowed(true); [...] // raw idle connection should now be closed // FIXME should be assertTrue assertFalse(rawIdleConnection.isClosed()); [...] // both wrapper and raw active connection should be closed assertTrue(activeConnection.isClosed()); assertTrue(rawActiveConnection.isClosed()); }

From my point of view and regarding the comment the assertFalse(rawIdleConnection.isClosed()); sould become assertTrue(rawIdleConnection.isClosed());.

Moreover, the next test method fails.
// Bugzilla Bug 28251: Returning dead database connections to BasicDataSource
// isClosed() failure blocks returning a connection to the pool
public void testIsClosedFailure() throws SQLException {
ds.setAccessToUnderlyingConnectionAllowed(true);
Connection conn = ds.getConnection();
assertNotNull(conn);
assertEquals(1, ds.getNumActive());

// set an IO failure causing the isClosed mathod to fail
TesterConnection tconn = (TesterConnection) ((DelegatingConnection)conn).getInnermostDelegate();
tconn.setFailure(new IOException("network error"));

try

{ conn.close(); fail("Expected SQLException"); }

catch(SQLException ex) { }

assertEquals(0, ds.getNumActive());
}
But again, looking at the AddObjectToPool of the GenericObjectPool (commons-pool), the IOException("Network error") is catch during the passivation. It implies that the underlying connection (in error) is not returned to the pool. It will be re-created by the evict thread if minIdle property is set.
Am I wrong ?

Sorry for the little long post.
Kind regards,
Jean-Louis MONTEIRO