1 files changed, 54 insertions, 49 deletions

diff --git a/cpp/src/Freeze/TransactionI.cpp b/cpp/src/Freeze/TransactionI.cpp
index e5b28a79132..f7844748ed8 100644
--- a/cpp/src/Freeze/TransactionI.cpp
+++ b/cpp/src/Freeze/TransactionI.cpp
@@ -45,20 +45,21 @@ Freeze::TransactionI::commit()
 
         if(_txTrace >= 1)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+            Trace out(_communicator->getLogger(), "Freeze.Transaction");
             out << "committed transaction " << hex << txnId << dec;
         }
     }
     catch(const ::DbDeadlockException& dx)
     {
-    
         if(_txTrace >= 1)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+            Trace out(_communicator->getLogger(), "Freeze.Transaction");
             out << "failed to commit transaction " << hex << txnId << dec << ": " << dx.what();
         }
 
         postCompletion(false, true);
+        // After postCompletion is called the transaction may be
+        // dead. Beware!
         DeadlockException ex(__FILE__, __LINE__);
         ex.message = dx.what();
         throw ex;
@@ -67,16 +68,20 @@ Freeze::TransactionI::commit()
     {
         if(_txTrace >= 1)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+            Trace out(_communicator->getLogger(), "Freeze.Transaction");
             out << "failed to commit transaction " << hex << txnId << dec << ": " << dx.what();
         }
 
         postCompletion(false, false);
+        // After postCompletion is called the transaction may be
+        // dead. Beware!
         DatabaseException ex(__FILE__, __LINE__);
         ex.message = dx.what();
         throw ex;
     }
     postCompletion(true, false);
+    // After postCompletion is called the transaction may be
+    // dead. Beware!
 }
 
 void
@@ -98,7 +103,7 @@ Freeze::TransactionI::rollback()
             
             if(_txTrace >= 1)
             {
-                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                Trace out(_communicator->getLogger(), "Freeze.Transaction");
                 out << "rolled back transaction " << hex << txnId << dec;
             }
         }
@@ -106,88 +111,70 @@ Freeze::TransactionI::rollback()
         {
             if(_txTrace >= 1)
             {
-                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                Trace out(_communicator->getLogger(), "Freeze.Transaction");
                 out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
             }
             
             postCompletion(false, true);
+            // After postCompletion is called the transaction may be
+            // dead. Beware!
             throw DeadlockException(__FILE__, __LINE__, dx.what());
         }
         catch(const ::DbException& dx)
         {
             if(_txTrace >= 1)
             {
-                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                Trace out(_communicator->getLogger(), "Freeze.Transaction");
                 out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
             }
             
             postCompletion(false, false);
+            // After postCompletion is called the transaction may be
+            // dead. Beware!
             throw DatabaseException(__FILE__, __LINE__, dx.what());
         }
         postCompletion(true, false);
+        // After postCompletion is called the transaction may be
+        // dead. Beware!
     }
 }
 
 Freeze::ConnectionPtr
 Freeze::TransactionI::getConnection() const
 {
-    if(_txn != 0)
-    {
-        return _connection;
-    }
-    else
-    {
-        return 0;
-    }
+    return _connection;
 }
 
 //
 // External refcount operations, from code holding a Transaction[I]Ptr
 //
 void
-Freeze::TransactionI::__incRef()
-{
-    Shared::__incRef();
-    if(_txn != 0)
-    {
-         _connection->__incRef();
-    }
-}
-
-void
 Freeze::TransactionI::__decRef()
 {
-    if(_txn != 0)
+    // 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)
     {
-         _connection->__decRef();
+        rb = true;
     }
     Shared::__decRef();
+    if(rb)
+    {
+        rollback();
+        // After this the transaction is dead.
+    }
 }
 
-//
-// The refcount on transaction held by the associated ConnectionI object
-//
-void
-Freeze::TransactionI::internalIncRef()
-{
-    Shared::__incRef();
-}
-
-void
-Freeze::TransactionI::internalDecRef()
-{
-    Shared::__incRef();
-}
-
-
 void
 Freeze::TransactionI::setPostCompletionCallback(const Freeze::PostCompletionCallbackPtr& cb)
 {
     _postCompletionCallback = cb;
 }
-
     
-Freeze::TransactionI::TransactionI(ConnectionI* connection) :
+Freeze::TransactionI::TransactionI(const ConnectionIPtr& connection) :
+    _communicator(connection->communicator()),
     _connection(connection),
     _txTrace(connection->txTrace()),
     _txn(0)
@@ -199,7 +186,7 @@ Freeze::TransactionI::TransactionI(ConnectionI* connection) :
         if(_txTrace >= 1)
         {
             long txnId = (_txn->id() & 0x7FFFFFFF) + 0x80000000L;
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+            Trace out(_communicator->getLogger(), "Freeze.Transaction");
             out << "started transaction " << hex << txnId << dec;
         }
     }
@@ -207,7 +194,7 @@ Freeze::TransactionI::TransactionI(ConnectionI* connection) :
     {
         if(_txTrace >= 1)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+            Trace out(_communicator->getLogger(), "Freeze.Transaction");
             out << "failed to start transaction: " << dx.what();
         }
 
@@ -228,7 +215,7 @@ Freeze::TransactionI::~TransactionI()
         }
         catch(const IceUtil::Exception& e)
         {
-            Error error(_connection->communicator()->getLogger());
+            Error error(_communicator->getLogger());
             error << "transaction rollback raised :" << e;
         }
     }
@@ -237,6 +224,10 @@ Freeze::TransactionI::~TransactionI()
 void
 Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
 {
+    // The order of assignment in this method is very important as
+    // calling both the post completion callback and
+    // Connection::clearTransaction may alter the transaction
+    // reference count which checks _txn.
     _txn = 0;
 
     if(_postCompletionCallback != 0)
@@ -244,5 +235,19 @@ Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
         _postCompletionCallback->postCompletion(committed, deadlock);
     }
 
-    _connection->clearTransaction(); // will release the internal refcount
+    // 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();
+    // At this point the transaction may be dead.
 }