http://bugzilla.zeroc.com/bugzilla/show_bug.cgi?id=2233

Squashed commit of the following:

commit 9c5b39640659633dfad70341c4e709eeda8d5f8e
Author: Matthew Newhook <matthew@zeroc.com>
Date:   Thu Sep 6 18:13:59 2007 +0800

    More fixes.

commit 49cade00fee4dfc00fd688ba1ca8bcbf80376b59
Author: Matthew Newhook <matthew@zeroc.com>
Date:   Thu Sep 6 17:07:36 2007 +0800

    removed debug.

commit f33af74a2fe64ee2433228133c58afe6278fc5f3
Author: Matthew Newhook <matthew@zeroc.com>
Date:   Thu Sep 6 17:03:01 2007 +0800

    fixes.

commit a145684f88b000087a6fe5551bb9018ad03c8307
Author: Matthew Newhook <matthew@zeroc.com>
Date:   Thu Sep 6 15:25:37 2007 +0800

    altered reference counting as suggested in the bug report.

9 files changed, 65 insertions, 38 deletions

diff --git a/cpp/config/PropertyNames.xml b/cpp/config/PropertyNames.xml
index cafd3f7d9c0..a9a320e7ba5 100755
--- a/cpp/config/PropertyNames.xml
+++ b/cpp/config/PropertyNames.xml
@@ -540,6 +540,7 @@ generated from the section label.
         <property name="Trace.Transaction" />
         <property name="Warn.CloseInFinalize" />
         <property name="Warn.Deadlocks" />
+        <property name="Warn.Rollback" />
     </section>
 
 </properties>
diff --git a/cpp/src/Freeze/ConnectionI.cpp b/cpp/src/Freeze/ConnectionI.cpp
index 66a1e8ba0f7..dae584e3bbd 100644
--- a/cpp/src/Freeze/ConnectionI.cpp
+++ b/cpp/src/Freeze/ConnectionI.cpp
@@ -46,7 +46,7 @@ Freeze::ConnectionI::close()
     {
         try
         {
-            _transaction->rollback();
+            _transaction->rollbackInternal(true);
         }
         catch(const DatabaseException&)
         {
@@ -77,6 +77,19 @@ Freeze::ConnectionI::getName() const
     return _envName;
 }
 
+//
+// External refcount operations, from code holding a Connection[I]Ptr
+//
+void
+Freeze::ConnectionI::__decRef()
+{
+    if(__getRef() == 2 && _transaction && _transaction->__getRef() == 1)
+    {
+        close();
+    }
+    Shared::__decRef();
+}
+
 Freeze::ConnectionI::~ConnectionI()
 {
     close();
diff --git a/cpp/src/Freeze/ConnectionI.h b/cpp/src/Freeze/ConnectionI.h
index aa4fb822089..31d8e206eb5 100644
--- a/cpp/src/Freeze/ConnectionI.h
+++ b/cpp/src/Freeze/ConnectionI.h
@@ -35,6 +35,11 @@ public:
 
     virtual std::string getName() const;
 
+    //
+    // Custom refcounting implementation
+    //
+    virtual void __decRef();
+
     virtual ~ConnectionI();
 
     ConnectionI(const SharedDbEnvPtr&);
diff --git a/cpp/src/Freeze/TransactionI.cpp b/cpp/src/Freeze/TransactionI.cpp
index f7844748ed8..d4e82e0d221 100644
--- a/cpp/src/Freeze/TransactionI.cpp
+++ b/cpp/src/Freeze/TransactionI.cpp
@@ -87,6 +87,22 @@ Freeze::TransactionI::commit()
 void
 Freeze::TransactionI::rollback()
 {
+    rollbackInternal(false);
+}
+
+Freeze::ConnectionPtr
+Freeze::TransactionI::getConnection() const
+{
+    if(_txn)
+    {
+        return _connection;
+    }
+    return 0;
+}
+
+void
+Freeze::TransactionI::rollbackInternal(bool warning)
+{
     if(_txn != 0)
     {
         long txnId = 0;
@@ -94,9 +110,15 @@ Freeze::TransactionI::rollback()
         {
             _connection->closeAllIterators();
             
-            if(_txTrace >= 1)
+            if(_txTrace >= 1 || (warning && _warnRollback))
             {
                 txnId = (_txn->id() & 0x7FFFFFFF) + 0x80000000L;
+                if(warning && _warnRollback)
+                {
+                    Warning warn(_communicator->getLogger());
+                    warn << "Freeze.Transaction: rolled back transaction " << hex << txnId << dec
+                         << " due to destruction.\nApplication code should explicitely call rollback or commit.";
+                }
             }
             
             _txn->abort();
@@ -139,32 +161,17 @@ Freeze::TransactionI::rollback()
     }
 }
 
-Freeze::ConnectionPtr
-Freeze::TransactionI::getConnection() const
-{
-    return _connection;
-}
-
 //
 // External refcount operations, from code holding a Transaction[I]Ptr
 //
 void
 Freeze::TransactionI::__decRef()
 {
-    // If dropping the second to last reference and there is still a
-    // transaction then this means the last reference is held by the
-    // connection. In this case we must rollback the transaction.
-    bool rb = false;
-    if(__getRef() == 2 && _txn)
+    if(__getRef() == 2 && _txn && _connection->__getRef() == 1)
     {
-        rb = true;
+        rollbackInternal(true);
     }
     Shared::__decRef();
-    if(rb)
-    {
-        rollback();
-        // After this the transaction is dead.
-    }
 }
 
 void
@@ -173,10 +180,17 @@ Freeze::TransactionI::setPostCompletionCallback(const Freeze::PostCompletionCall
     _postCompletionCallback = cb;
 }
     
-Freeze::TransactionI::TransactionI(const ConnectionIPtr& connection) :
+//
+// The constructor takes a ConnectionI* instead of a ConnectionIPtr
+// because we have to ensure there is no call to __decRef while the
+// transaction or the connection are not assigned to a Ptr in
+// user-code.
+//
+Freeze::TransactionI::TransactionI(ConnectionI* connection) :
     _communicator(connection->communicator()),
     _connection(connection),
     _txTrace(connection->txTrace()),
+    _warnRollback(_communicator->getProperties()->getPropertyAsIntWithDefault("Freeze.Warn.Rollback", 1)),
     _txn(0)
 {
     try
@@ -235,19 +249,6 @@ Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
         _postCompletionCallback->postCompletion(committed, deadlock);
     }
 
-    // Its necessary here to copy the connection before calling
-    // clearTransaction because this may release the last reference. This specifically
-    // occurs in the following scenario:
-    //
-    // TransactionalEvictorContext holds the _tx. It calls
-    // _tx->commit(). This comes into this method, and calls
-    // _postCompletionCallback. This causes the context to drop the
-    // _tx reference (reference count is now 1). The
-    // connection->clearTransaction() is then called which drops its
-    // reference causing the transaction to be deleted.
-    //
-    ConnectionIPtr con = _connection;
-    _connection = 0; // Drop the connection
-    con->clearTransaction();
+    _connection->clearTransaction();
     // At this point the transaction may be dead.
 }
diff --git a/cpp/src/Freeze/TransactionI.h b/cpp/src/Freeze/TransactionI.h
index a44e782bbc0..cb5da8a4e61 100644
--- a/cpp/src/Freeze/TransactionI.h
+++ b/cpp/src/Freeze/TransactionI.h
@@ -36,6 +36,7 @@ public:
 
     virtual void rollback();
 
+
     virtual ConnectionPtr getConnection() const;
     
     //
@@ -43,9 +44,10 @@ public:
     //
     virtual void __decRef();
 
+    void rollbackInternal(bool);
     void setPostCompletionCallback(const PostCompletionCallbackPtr&);
 
-    TransactionI(const ConnectionIPtr&);
+    TransactionI(ConnectionI*);
     ~TransactionI();
     
     DbTxn*
@@ -63,6 +65,7 @@ private:
     const Ice::CommunicatorPtr _communicator;
     ConnectionIPtr _connection;
     const Ice::Int _txTrace;
+    const Ice::Int _warnRollback;
     DbTxn* _txn;
     PostCompletionCallbackPtr _postCompletionCallback;
 };
diff --git a/cpp/src/FreezeScript/transformdb.cpp b/cpp/src/FreezeScript/transformdb.cpp
index 150235e3ea1..566bb4a01e2 100644
--- a/cpp/src/FreezeScript/transformdb.cpp
+++ b/cpp/src/FreezeScript/transformdb.cpp
@@ -837,6 +837,9 @@ run(int argc, char** argv, const Ice::CommunicatorPtr& communicator)
             status = EXIT_FAILURE;
         }
     }
+    // Clear the transaction before closing the database environment.
+    txNew = 0;
+
     dbEnv.close(0);
     dbEnvNew.close(0);
 
diff --git a/cpp/src/Ice/PropertyNames.cpp b/cpp/src/Ice/PropertyNames.cpp
index 92d94b02ec2..b4eb08cafce 100644
--- a/cpp/src/Ice/PropertyNames.cpp
+++ b/cpp/src/Ice/PropertyNames.cpp
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Thu Aug 30 13:57:41 2007
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Thu Sep  6 17:17:28 2007
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -559,6 +559,7 @@ const IceInternal::Property FreezePropsData[] =
     IceInternal::Property("Freeze.Trace.Transaction", false, 0),
     IceInternal::Property("Freeze.Warn.CloseInFinalize", false, 0),
     IceInternal::Property("Freeze.Warn.Deadlocks", false, 0),
+    IceInternal::Property("Freeze.Warn.Rollback", false, 0),
 };
 
 const IceInternal::PropertyArray
diff --git a/cpp/src/Ice/PropertyNames.h b/cpp/src/Ice/PropertyNames.h
index c4fae4b8cbd..2085a9ea8bc 100644
--- a/cpp/src/Ice/PropertyNames.h
+++ b/cpp/src/Ice/PropertyNames.h
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Thu Aug 30 13:57:41 2007
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Thu Sep  6 17:17:28 2007
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
diff --git a/cpp/test/Freeze/dbmap/run.py b/cpp/test/Freeze/dbmap/run.py
index 5b44fac78fc..6fa90103510 100755
--- a/cpp/test/Freeze/dbmap/run.py
+++ b/cpp/test/Freeze/dbmap/run.py
@@ -29,7 +29,7 @@ TestUtil.cleanDbDir(dbdir)
 client = os.path.join(testdir, "client")
 
 print "starting client...",
-clientPipe = os.popen(client + TestUtil.clientOptions + " " + testdir + " 2>&1")
+clientPipe = os.popen(client + TestUtil.clientOptions + " --Freeze.Warn.Rollback=0 " + testdir + " 2>&1")
 print "ok"
 
 TestUtil.printOutputFromPipe(clientPipe)