Commons Dbcp
  1. Commons Dbcp
  2. DBCP-269

final jdbc connection never closed --> number of connections grows

    Details

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

      Windows, JDK 5

      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

      1. patch.txt
        0.7 kB
        Jean-Louis MONTEIRO

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        4d 11h 3m 1 Dain Sundstrom 16/Jun/08 21:26
        Dain Sundstrom made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 1.3 [ 12311977 ]
        Hide
        Dain Sundstrom added a comment -

        I didn't use the patch as is. Instead of changing the behavior of the proxy, I changed the close logic to inspect the underlying connection to determine if the proxy should be returned to the pool or destroyed.

        Show
        Dain Sundstrom added a comment - I didn't use the patch as is. Instead of changing the behavior of the proxy, I changed the close logic to inspect the underlying connection to determine if the proxy should be returned to the pool or destroyed.
        Dain Sundstrom made changes -
        Assignee Dain Sundstrom [ dain ]
        Jean-Louis MONTEIRO made changes -
        Field Original Value New Value
        Attachment patch.txt [ 12383900 ]
        Jean-Louis MONTEIRO created issue -

          People

          • Assignee:
            Dain Sundstrom
            Reporter:
            Jean-Louis MONTEIRO
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development