castor

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. Text File
    DatabaseImpl.java.diff.txt
    14/Mar/06 12:16 AM
    1 kB
    Bruce Snyder
  2. Text File
    patch-C1356-20060316.txt
    16/Mar/06 5:39 PM
    5 kB
    Ralf Joachim

Issue Links

depends upon

Improvement - An improvement or enhancement to an existing feature or task. CASTOR-1345 Refactor DatabaseImpl

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Werner Guttmann added a comment -

I am currently in the progress of finalizing a patch that refactores DatabaseImpl into something more meaningful, trying to cleanup that if (transaction == null) business and replacing it with a proper strategy. Can we please delay this until I have committed CASTOR-1345 ?

In addition, I believe that the patch supplied (whilst dealing with a symptom) does not really tackle the root cause of this problem. I agree that above scenario should not happen at all, and that we should try to fix this. But first I need to dig up an existing Jira entry that provides a detailed description of the underlying (read real) problem, and take it from there.

Show
Werner Guttmann added a comment - I am currently in the progress of finalizing a patch that refactores DatabaseImpl into something more meaningful, trying to cleanup that if (transaction == null) business and replacing it with a proper strategy. Can we please delay this until I have committed CASTOR-1345 ? In addition, I believe that the patch supplied (whilst dealing with a symptom) does not really tackle the root cause of this problem. I agree that above scenario should not happen at all, and that we should try to fix this. But first I need to dig up an existing Jira entry that provides a detailed description of the underlying (read real) problem, and take it from there.
Hide
Permalink
Bruce Snyder added a comment -

Is this the existing JIRA issue to which you're referring: CASTOR-630

Show
Bruce Snyder added a comment - Is this the existing JIRA issue to which you're referring: CASTOR-630
Hide
Permalink
Werner Guttmann added a comment -

No, Bruce, I am referring to CASTOR-1345, as that's where we have refactored DatabaseImpl by splitting it up into LocalDatabaseImpl, GlobalDatabaseImpl and an abstract base class. I will commit this latr today ... so kplease bear with me for the time being.

Show
Werner Guttmann added a comment - No, Bruce, I am referring to CASTOR-1345 , as that's where we have refactored DatabaseImpl by splitting it up into LocalDatabaseImpl, GlobalDatabaseImpl and an abstract base class. I will commit this latr today ... so kplease bear with me for the time being.
Hide
Permalink
Ralf Joachim added a comment -

I guess Bruce refered to the last sentens of your comment: But first I need to dig up an existing Jira entry that provides a detailed description of the underlying (read real) problem, and take it from there.

Show
Ralf Joachim added a comment - I guess Bruce refered to the last sentens of your comment: But first I need to dig up an existing Jira entry that provides a detailed description of the underlying (read real) problem, and take it from there.
Hide
Permalink
Werner Guttmann added a comment -

Oops, sorry, yes, this is one of the existing bug reports that describes the problem and suggest a solution. As already said, I think we should apply the patch as provided to minimize the damage, but at the same time keep an eye on the real (underlying) problem.

Show
Werner Guttmann added a comment - Oops, sorry, yes, this is one of the existing bug reports that describes the problem and suggest a solution. As already said, I think we should apply the patch as provided to minimize the damage, but at the same time keep an eye on the real (underlying) problem.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Ralf Joachim added a comment -

Final patch.

Show
Ralf Joachim added a comment - Final patch.

People

  • Assignee:
    Werner Guttmann
    Reporter:
    Bruce Snyder
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: