Commons Dbcp
  1. Commons Dbcp
  2. DBCP-372

Statement Leak occurs when batch update is used.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.3, 1.4
    • Fix Version/s: 1.5.1, 2.0
    • Labels:
      None
    • Environment:

      Oracle 11g

      Description

      org.apache.commons.dbcp.PoolablePreparedStatement#passivate()
      execute clearBatch().
      (DBCP-264)

      But this clearBatch() throw SQLException.
      (DelegatingStatement#checkOpen() throw SQLException, because _closed is true.)

      The result,
      the PoolablePreparedStatement doesn't return to pool, and
      the PoolablePreparedStatement doesn't execute PreparedStatement#close().

      When a lot of data is processed,
      in the case of Oracle

      • ORA-00604
      • ORA-01000

      occurs.

      Proposal:
      "clearBatch();" in passivate() method
      changes as follows.

      batchAdded = false;
      getInnermostDelegate().clearBatch();

        Issue Links

          Activity

          Hide
          Thomas Neidhart added a comment -

          Indeed, the current passivate method in PoolablePreparedStatement does the following:

              public void passivate() throws SQLException {
                  _closed = true;
                  if(_conn != null) {
                      _conn.removeTrace(this);
                  }
          
                  // The JDBC spec requires that a statement closes any open
                  // ResultSet's when it is closed.
                  // FIXME The PreparedStatement we're wrapping should handle this for us.
                  // See bug 17301 for what could happen when ResultSets are closed twice.
                  List<AbandonedTrace> resultSets = getTrace();
                  if( resultSets != null) {
                      ResultSet[] set = resultSets.toArray(new ResultSet[resultSets.size()]);
                      for (int i = 0; i < set.length; i++) {
                          set[i].close();
                      }
                      clearTrace();
                  }
                  if (batchAdded) {
                      clearBatch();
                  }
                  
                  super.passivate();
              }
          

          Thus, indicating that the connection is closed first, and then afterwards trying to clear any batches. Now the clearBatch method throws an exception if the connection is already marked as closed.

          Imho, the suggested fix would be fine, but we could also change the passivate method to call clearBatch before setting _closed to true.

          Show
          Thomas Neidhart added a comment - Indeed, the current passivate method in PoolablePreparedStatement does the following: public void passivate() throws SQLException { _closed = true; if(_conn != null) { _conn.removeTrace(this); } // The JDBC spec requires that a statement closes any open // ResultSet's when it is closed. // FIXME The PreparedStatement we're wrapping should handle this for us. // See bug 17301 for what could happen when ResultSets are closed twice. List<AbandonedTrace> resultSets = getTrace(); if( resultSets != null) { ResultSet[] set = resultSets.toArray(new ResultSet[resultSets.size()]); for (int i = 0; i < set.length; i++) { set[i].close(); } clearTrace(); } if (batchAdded) { clearBatch(); } super.passivate(); } Thus, indicating that the connection is closed first, and then afterwards trying to clear any batches. Now the clearBatch method throws an exception if the connection is already marked as closed. Imho, the suggested fix would be fine, but we could also change the passivate method to call clearBatch before setting _closed to true.
          Hide
          Phil Steitz added a comment -

          Any other comments / suggestions on this? It might also be good to wrap and swallow SQLException per the first patch for DBCP-264 (along with Thomas' suggested changes).

          Show
          Phil Steitz added a comment - Any other comments / suggestions on this? It might also be good to wrap and swallow SQLException per the first patch for DBCP-264 (along with Thomas' suggested changes).
          Hide
          Mark Thomas added a comment -

          Thanks for the report and the fix. Sorry it has taken so long to address this.

          I think, based on my testing, that the statement leak has been fixed by changes to Pool but that prior to this fix the problem manifested itself as the PreparedStatement always being invalidated on return to the pool (making the pooling of statements pointless) if batchUpdate() was used.

          I've fixed trunk for the 2.x series the 1.5.x branch. It will be included in the next release of each.

          If there are further 1.3.x and 1.4.x releases (currently being discussed on the dev mailing list) my expectation is that they will be generated from the 1.5.x branch so will pick up this fix too.

          Show
          Mark Thomas added a comment - Thanks for the report and the fix. Sorry it has taken so long to address this. I think, based on my testing, that the statement leak has been fixed by changes to Pool but that prior to this fix the problem manifested itself as the PreparedStatement always being invalidated on return to the pool (making the pooling of statements pointless) if batchUpdate() was used. I've fixed trunk for the 2.x series the 1.5.x branch. It will be included in the next release of each. If there are further 1.3.x and 1.4.x releases (currently being discussed on the dev mailing list) my expectation is that they will be generated from the 1.5.x branch so will pick up this fix too.

            People

            • Assignee:
              Unassigned
              Reporter:
              Naozumi Taromaru
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development