castor
  1. castor
  2. CASTOR-1356

DatabaseImpl.finalize and false alarm log message

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0 M3
    • Fix Version/s: 1.0 M4
    • Component/s: JDO
    • Labels:
      None
    • Number of attachments :
      2

      Description

      Upon performing a begin and commit/rollback on a local transaction around every entity bean call that isn't taking place in an official JTA transaction, the OpenEJB team sees the following:

      Mar 9, 2006 11:19:48 AM org.exolab.castor.jdo.engine.DatabaseImpl.finalize
      WARNING: An open database instance
      (org.exolab.castor.jdo.engine.DatabaseImpl@a8f250:Local_TX_Database) against database: Local_TX_Database has been encountered. This instance will be closed now to release system resources. Please consider changing your code as well to enforce that all database connections are closed after use.SQL executed, but not closed:null

      The code that does that is here:

      http://svn.openejb.org/trunk/openejb3/container/openejb-core/src/main/java/org/openejb/alt/containers/castor_cmp11/CastorCmpEntityTxPolicy.java

      OpenEJB is using local transactions which is LocalTransactionContext and:

      1. The connection is closed immediately after commit()/rollback()
      2. But the closeConnections() method is an empty, no-op impl.

      So the DatabaseImpl.finalize method checks to see if you are using LocalTransactionContext and makes a point to reprimand you for not calling close, then the closeConnections() method of LocalTransactionContext does nothing anyway.

      If you call commit/rollback responsibly on a DatabaseImpl with a LocalTransactionContext, calling close and the log message are unnecessary. Strangely, OpenEJB even runs into thread deadlock issues upon adding a close call after db.commit/rollback.

      Attached is a patch that will prevent this warning from being output unnecessarily.

      1. DatabaseImpl.java.diff.txt
        1 kB
        Bruce Snyder
      2. patch-C1356-20060316.txt
        5 kB
        Ralf Joachim

        Issue Links

          Activity

          Hide
          Werner Guttmann added a comment -

          Just looking at the afterInvoke() method of

          http://svn.openejb.org/trunk/openejb3/container/openejb-core/src/main/java/org/openejb/alt/containers/castor_cmp11/CastorCmpEntityTxPolicy.java

          I wonder whether there's any reason not to call Database.close() in the finally clause of this method ?

          Show
          Werner Guttmann added a comment - Just looking at the afterInvoke() method of http://svn.openejb.org/trunk/openejb3/container/openejb-core/src/main/java/org/openejb/alt/containers/castor_cmp11/CastorCmpEntityTxPolicy.java I wonder whether there's any reason not to call Database.close() in the finally clause of this method ?
          Hide
          David Blevins added a comment -

          I tried that before taking this up with Bruce. We had already been encountering occasional thread deadlocks around CMP calls and adding close after commits and rollback calls in these methods seemed to make deadlocks a norm.
          afterInvoke()
          handleApplicationException()
          handleSystemException()

          We only use the Database.begin/commit/rollback calls when we need a Local Transaction (i.e. no JTA transaction is in progress), so we know we are always dealing with the LocalTransactionContext which does all the required cleanup on commit/rollback. Synchronized calls are not cheap and we are already paying for one on a guaranteed call to either commit or rollback, it seems silly to be obligated to make a second synchronized call to close when it does nothing for a LocalTransactionContext.

          Similarly, we want to avoid the sychronized call to close() in the finalize Thread for Database instances using a LocalTransactionContext that is no longer open.

          Show
          David Blevins added a comment - I tried that before taking this up with Bruce. We had already been encountering occasional thread deadlocks around CMP calls and adding close after commits and rollback calls in these methods seemed to make deadlocks a norm. afterInvoke() handleApplicationException() handleSystemException() We only use the Database.begin/commit/rollback calls when we need a Local Transaction (i.e. no JTA transaction is in progress), so we know we are always dealing with the LocalTransactionContext which does all the required cleanup on commit/rollback. Synchronized calls are not cheap and we are already paying for one on a guaranteed call to either commit or rollback, it seems silly to be obligated to make a second synchronized call to close when it does nothing for a LocalTransactionContext. Similarly, we want to avoid the sychronized call to close() in the finalize Thread for Database instances using a LocalTransactionContext that is no longer open.
          Hide
          Werner Guttmann added a comment -

          David, I assume you noted that Database.close() is not called as part of issueing a Database.rollback() ? Other than that, let's commit this patch.

          Show
          Werner Guttmann added a comment - David, I assume you noted that Database.close() is not called as part of issueing a Database.rollback() ? Other than that, let's commit this patch.
          Hide
          Ralf Joachim added a comment -

          I suggest to change

          if (_scope != null || !_ctx.isOpen()) {

          to

          if (_scope != null || _ctx == null || !_ctx.isOpen()) {

          which is similar to

          if (_scope != null || !isActive()) {

          to prevent NullPointerExeption's.

          Show
          Ralf Joachim added a comment - I suggest to change if (_scope != null || !_ctx.isOpen()) { to if (_scope != null || _ctx == null || !_ctx.isOpen()) { which is similar to if (_scope != null || !isActive()) { to prevent NullPointerExeption's.
          Hide
          Ralf Joachim added a comment -

          Final patch.

          Show
          Ralf Joachim added a comment - Final patch.

            People

            • Assignee:
              Werner Guttmann
              Reporter:
              Bruce Snyder
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: