Derby
  1. Derby
  2. DERBY-3319

Logical connections do not check if a transaction is active on close

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.2.1, 10.4.1.3, 10.5.1.1
    • Fix Version/s: 10.5.1.1
    • Component/s: JDBC, Network Client
    • Labels:
      None
    • Environment:
      Embedded driver and client driver.
    • Issue & fix info:
      Release Note Needed

      Description

      If you call close on a logical connection, for instance as obtained through a PooledConnection, it does not check if there is an active transaction.
      The close of the logical connection is allowed, and even the close of the parent PooledConnection is allowed in the client driver. This can/will cause resources to be left on the server, and later operations might fail (typically with lock timeouts because the "closed" transaction is still holding locks).
      I do not know if gc will solve this eventually, but I would say the current behavior of the client driver is wrong in any case.
      There is difference in the behavior between the embedded and the client driver, and there also seems to be a bug in the embedded driver.

      The analysis above is a bit sketchy, so it might be required to look into the issue a bit more...
      I will attach a repro (JDBC usage should be verified as well, is it legal / as intended?)

      1. d3319-1a.diff
        18 kB
        Knut Anders Hatlen
      2. d3319-1a.stat
        0.7 kB
        Knut Anders Hatlen
      3. LogicalConnectionCloseActiveTransactionBug.java
        2 kB
        Kristian Waagan
      4. releaseNote.html
        5 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          'LogicalConnectionCloseActiveTransactionBug.java' is a repro for this issue.
          Note the system property for running with the embedded or the client driver. Read class comment for how to compile/run.

          Show
          Kristian Waagan added a comment - 'LogicalConnectionCloseActiveTransactionBug.java' is a repro for this issue. Note the system property for running with the embedded or the client driver. Read class comment for how to compile/run.
          Hide
          Kristian Waagan added a comment -

          A test in jdbcapi.StatementPoolingTest has been disabled due to this bug:
          // This test fails, DERBY-3319 is probably the cause.
          //reqDataSuite.addTest(new StatementPoolingTest(
          // "resTestNoDataCommittedOnInvalidTransactionState"));

          It should be enabled and verified when this bug has been fixed.

          Show
          Kristian Waagan added a comment - A test in jdbcapi.StatementPoolingTest has been disabled due to this bug: // This test fails, DERBY-3319 is probably the cause. //reqDataSuite.addTest(new StatementPoolingTest( // "resTestNoDataCommittedOnInvalidTransactionState")); It should be enabled and verified when this bug has been fixed.
          Hide
          Knut Anders Hatlen added a comment -

          Let's see if I understand this issue correctly. Based on experiments with the attached test case, I think this is how the drivers behave:

          1. close() on logical connections with active transactions does not fail and resources are not freed (both embedded and client)

          2. EmbedPooledConnection.getConnection() issues a rollback on the physical connection

          3. ClientPooledConnection.getConnection() does not issue a rollback on the physical connection if the previous logical connection has been closed

          4. ClientPooledConnection.getConnection() issues a rollback on the physical connection if the previous logical connection has not been closed

          Does that sound about right?

          Show
          Knut Anders Hatlen added a comment - Let's see if I understand this issue correctly. Based on experiments with the attached test case, I think this is how the drivers behave: 1. close() on logical connections with active transactions does not fail and resources are not freed (both embedded and client) 2. EmbedPooledConnection.getConnection() issues a rollback on the physical connection 3. ClientPooledConnection.getConnection() does not issue a rollback on the physical connection if the previous logical connection has been closed 4. ClientPooledConnection.getConnection() issues a rollback on the physical connection if the previous logical connection has not been closed Does that sound about right?
          Hide
          Kristian Waagan added a comment -

          I would think points 1 is incorrect behavior. If that one is fixed, I think point 3 might be okay.

          Points 2 and 4 are good. I believe it is required to be able to support "connection stealing", i.e. when a connection pool implementation executes a sequence like this:
          PooledConnection pc = ...
          Connection lc1 = pc.getConnection()
          // Give lc1 to a user/client.
          // Then wait, and determine that the user having lc1 has crashed / timed out / misbehaved.
          // Take back control of the physical connection and give a new logical connection to another user.
          Connection lc2 = pc.getConnection()
          // lc1 will now be closed / invalidated and should not contain any reference to the pooled / physical connection.

          We should not throw an exception in this case. Also note that the reference to the PooledConnection will normally not be available to end-users.
          I logged this issue to get point 1 fixed.

          Show
          Kristian Waagan added a comment - I would think points 1 is incorrect behavior. If that one is fixed, I think point 3 might be okay. Points 2 and 4 are good. I believe it is required to be able to support "connection stealing", i.e. when a connection pool implementation executes a sequence like this: PooledConnection pc = ... Connection lc1 = pc.getConnection() // Give lc1 to a user/client. // Then wait, and determine that the user having lc1 has crashed / timed out / misbehaved. // Take back control of the physical connection and give a new logical connection to another user. Connection lc2 = pc.getConnection() // lc1 will now be closed / invalidated and should not contain any reference to the pooled / physical connection. We should not throw an exception in this case. Also note that the reference to the PooledConnection will normally not be available to end-users. I logged this issue to get point 1 fixed.
          Hide
          Knut Anders Hatlen added a comment -

          Regarding the "connection stealing", it seems like that's what ClientPooledConnection, ClientXAConnection and EmbedPooledConnection do. However, EmbedXAConnection only steals the connection if it is not part of a global connection. If it is part of a global XA connection, an exception is thrown. Here's the relevant code and comment in EmbedXAConnection.getConnection():

          if (currentConnectionHandle != null)

          { // this can only happen if someone called start(Xid), // getConnection, getConnection (and we are now the 2nd // getConnection call). // Cannot yank a global connection away like, I don't think... throw Util.generateCsSQLException( SQLState.CANNOT_CLOSE_ACTIVE_XA_CONNECTION); }

          I'm not sure what the correct approach is in this case. For pooled connections, I see that it makes sense to allow stealing the connection and aborting the active transaction because of the reasons Kristian mentioned above, but it's not clear to me if the same would apply to global XA connections.

          One difference between pooled connections and XA connections in this respect is that PooledConnection.getConnection() is expected to return a fresh logical connection that executes in a different transaction than the previous logical connection, whereas the connection returned by XAConnection.getConnection() is expected to execute in the same global transaction as the previous logical connection did. At least, J2EEDataSourceTest is testing that they behave this way, so I assume that it is expected.

          Show
          Knut Anders Hatlen added a comment - Regarding the "connection stealing", it seems like that's what ClientPooledConnection, ClientXAConnection and EmbedPooledConnection do. However, EmbedXAConnection only steals the connection if it is not part of a global connection. If it is part of a global XA connection, an exception is thrown. Here's the relevant code and comment in EmbedXAConnection.getConnection(): if (currentConnectionHandle != null) { // this can only happen if someone called start(Xid), // getConnection, getConnection (and we are now the 2nd // getConnection call). // Cannot yank a global connection away like, I don't think... throw Util.generateCsSQLException( SQLState.CANNOT_CLOSE_ACTIVE_XA_CONNECTION); } I'm not sure what the correct approach is in this case. For pooled connections, I see that it makes sense to allow stealing the connection and aborting the active transaction because of the reasons Kristian mentioned above, but it's not clear to me if the same would apply to global XA connections. One difference between pooled connections and XA connections in this respect is that PooledConnection.getConnection() is expected to return a fresh logical connection that executes in a different transaction than the previous logical connection, whereas the connection returned by XAConnection.getConnection() is expected to execute in the same global transaction as the previous logical connection did. At least, J2EEDataSourceTest is testing that they behave this way, so I assume that it is expected.
          Hide
          Knut Anders Hatlen added a comment -

          Currently, plain connections (from DriverManager) throw an exception on close if they have uncommitted operations and they have auto-commit off. I think connections that come from data sources should behave the same way, but if the connection is associated with an XA transaction, I think it should not throw an exception because

          a) the connection object doesn't control commit/rollback. Even if the connection is closed, the global transaction can still be committed or rolled back with the XAResource. I think the rationale for disallowing close on plain connections when the transaction is active, is that there's no way to end the transaction after the connection is closed. Since this is not the case for XA transactions, there's no need to throw the exception.

          b) there are regression tests that depend on the ability to close a connection before ending the associated XA transaction (sequences like XAResource.start() ... XAConnection.getConnection() ... Connection.close() ... XAConnection.getConnection() ... Connection.close() ... XAResource.end()) so keeping the current behaviour for these connections sounds safer

          Unless there are objections to this approach, I'll post a patch which implements this new behaviour.

          Show
          Knut Anders Hatlen added a comment - Currently, plain connections (from DriverManager) throw an exception on close if they have uncommitted operations and they have auto-commit off. I think connections that come from data sources should behave the same way, but if the connection is associated with an XA transaction, I think it should not throw an exception because a) the connection object doesn't control commit/rollback. Even if the connection is closed, the global transaction can still be committed or rolled back with the XAResource. I think the rationale for disallowing close on plain connections when the transaction is active, is that there's no way to end the transaction after the connection is closed. Since this is not the case for XA transactions, there's no need to throw the exception. b) there are regression tests that depend on the ability to close a connection before ending the associated XA transaction (sequences like XAResource.start() ... XAConnection.getConnection() ... Connection.close() ... XAConnection.getConnection() ... Connection.close() ... XAResource.end()) so keeping the current behaviour for these connections sounds safer Unless there are objections to this approach, I'll post a patch which implements this new behaviour.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch that changes the behaviour as described in the previous
          comment. The patch touches the following files:

          Embedded:

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java

          • factored out code to check for active transaction in close() so
            that it can be used in EmbedPooledConnection too

          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnectionControl.java

          • new interface method to check if it is allowed to close a brokered
            connection (checkClose())

          M java/engine/org/apache/derby/jdbc/EmbedPooledConnection.java

          • implementation of BrokeredConnectionControl.checkClose()
          • in getConnection(), reset the physical connection before closing
            the existing logical connection, so that an exception isn't thrown
            if the logical connection is still active

          M java/engine/org/apache/derby/jdbc/EmbedXAConnection.java

          • factored out common code to check if the transaction is global
            into a helper method (isGlobal())
          • implementation of BrokeredConnectionControl.checkClose()

          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java

          • use BrokeredConnectionControl.checkClose() in close() to get the
            expected exception when the transaction is active

          Client:

          M java/client/org/apache/derby/client/net/NetConnection.java

          • fixed existing method (allowCloseInUOW_()) for checking if the
            connection can be closed in unit of work (previously is always
            returned false)

          M java/client/org/apache/derby/client/am/Connection.java

          • transactionInProgress(): check NetConnection.allowCloseInUOW_()
            instead of the autoCommit_ flag to find out if a transaction in
            progress prevents us from closing the connection. Without this
            change, XAConnection.getConnection() thinks that it needs to roll
            back active global XA transactions, which it is not allowed to do.
          • checkForTransactionInProgress(): don't check allowCloseInUOW_()
            since it is now checked in transactionInProgress()

          M java/client/org/apache/derby/client/am/LogicalConnection.java

          • call checkForTransactionInProgress() from close() to get the
            expected exception when the transaction is active

          Tests:

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementPoolingTest.java

          • enabled the test case that had been disabled because of this issue
          • fixed two occurrences of connections being closed with active
            transactions

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/J2EEDataSourceTest.java

          • test cases for close of active connections with all combinations of
            { auto-commit, no auto-commit }

            ×

            { DataSource, ConnectionPoolDS, local XADataSource, global XADataSource}

            ×

            { client, embedded }

          All the regression tests passed, except for the wisconsin test, which
          passed on rerun.

          Show
          Knut Anders Hatlen added a comment - Here's a patch that changes the behaviour as described in the previous comment. The patch touches the following files: Embedded: M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java factored out code to check for active transaction in close() so that it can be used in EmbedPooledConnection too M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnectionControl.java new interface method to check if it is allowed to close a brokered connection (checkClose()) M java/engine/org/apache/derby/jdbc/EmbedPooledConnection.java implementation of BrokeredConnectionControl.checkClose() in getConnection(), reset the physical connection before closing the existing logical connection, so that an exception isn't thrown if the logical connection is still active M java/engine/org/apache/derby/jdbc/EmbedXAConnection.java factored out common code to check if the transaction is global into a helper method (isGlobal()) implementation of BrokeredConnectionControl.checkClose() M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java use BrokeredConnectionControl.checkClose() in close() to get the expected exception when the transaction is active Client: M java/client/org/apache/derby/client/net/NetConnection.java fixed existing method (allowCloseInUOW_()) for checking if the connection can be closed in unit of work (previously is always returned false) M java/client/org/apache/derby/client/am/Connection.java transactionInProgress(): check NetConnection.allowCloseInUOW_() instead of the autoCommit_ flag to find out if a transaction in progress prevents us from closing the connection. Without this change, XAConnection.getConnection() thinks that it needs to roll back active global XA transactions, which it is not allowed to do. checkForTransactionInProgress(): don't check allowCloseInUOW_() since it is now checked in transactionInProgress() M java/client/org/apache/derby/client/am/LogicalConnection.java call checkForTransactionInProgress() from close() to get the expected exception when the transaction is active Tests: M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementPoolingTest.java enabled the test case that had been disabled because of this issue fixed two occurrences of connections being closed with active transactions M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/J2EEDataSourceTest.java test cases for close of active connections with all combinations of { auto-commit, no auto-commit } × { DataSource, ConnectionPoolDS, local XADataSource, global XADataSource} × { client, embedded } All the regression tests passed, except for the wisconsin test, which passed on rerun.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 675870.

          I think a release note is needed, since now we'll throw an exception where we previously didn't, so I'm leaving the issue open for now.

          Show
          Knut Anders Hatlen added a comment - Committed revision 675870. I think a release note is needed, since now we'll throw an exception where we previously didn't, so I'm leaving the issue open for now.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a release note for this issue.

          Show
          Knut Anders Hatlen added a comment - Attaching a release note for this issue.
          Hide
          Dag H. Wanvik added a comment -

          Release note looks good to me.

          Show
          Dag H. Wanvik added a comment - Release note looks good to me.
          Hide
          Kristian Waagan added a comment -

          Closing issue.

          I verified that the repro no longer failed with trunk, and that it did indeed fail with 10.4 (that is, it threw an exception in a different place - when closing the last physical PooledConnection).
          The release notes also look good.

          Show
          Kristian Waagan added a comment - Closing issue. I verified that the repro no longer failed with trunk, and that it did indeed fail with 10.4 (that is, it threw an exception in a different place - when closing the last physical PooledConnection). The release notes also look good.
          Hide
          Kathey Marsden added a comment -

          In addition to DERBY-4053, I think we had a user report of a hard to diagnose issue after this change. To facilitate diagnosis, should the error that the connection cannot be closed be logged to derby.log especially if derby.stream.error.logSeverityLevel=0?

          Show
          Kathey Marsden added a comment - In addition to DERBY-4053 , I think we had a user report of a hard to diagnose issue after this change. To facilitate diagnosis, should the error that the connection cannot be closed be logged to derby.log especially if derby.stream.error.logSeverityLevel=0?
          Hide
          Kathey Marsden added a comment -

          Linking DERBY-4053.
          The new requirement that local transactions be rolled back before closing pooled connections was causing network server to fail to shutdown properly.

          Show
          Kathey Marsden added a comment - Linking DERBY-4053 . The new requirement that local transactions be rolled back before closing pooled connections was causing network server to fail to shutdown properly.
          Hide
          Knut Anders Hatlen added a comment -

          Kathey, since the error has statement severity (20000) it is worth filing a bug if it's not automatically logged with logSeverityLevel=0.

          Show
          Knut Anders Hatlen added a comment - Kathey, since the error has statement severity (20000) it is worth filing a bug if it's not automatically logged with logSeverityLevel=0.
          Hide
          Kathey Marsden added a comment -

          I raised the priority of DERBY-1191 for the general case and linked it to this issue. I think this particular case is pretty urgent as it seems really hard to track these issues down as we found in DERBY-4053 and DERBY-4225 but I don't know if it is easier just to fix the general case. If not a subtask of DERBY-1191 can be filed to fix this one.

          Show
          Kathey Marsden added a comment - I raised the priority of DERBY-1191 for the general case and linked it to this issue. I think this particular case is pretty urgent as it seems really hard to track these issues down as we found in DERBY-4053 and DERBY-4225 but I don't know if it is easier just to fix the general case. If not a subtask of DERBY-1191 can be filed to fix this one.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development