Merge branch 'master' of ssh://cvs.zeroc.com/home/git/ice

9 files changed, 158 insertions, 53 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..258060f9f5c 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,44 @@ Freeze::ConnectionI::getName() const
     return _envName;
 }
 
+void
+Freeze::ConnectionI::__incRef()
+{
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    _refCount++;
+}
+
+
+void
+Freeze::ConnectionI::__decRef()
+{
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    if(--_refCount == 0)
+    {
+        sync.release();
+        delete this;
+    }
+    else if(_refCount == 1 && _transaction != 0 && _transaction->dbTxn() != 0 && _transaction->__getRefNoSync() == 1)
+    {
+        sync.release();
+        close();
+    }
+}
+
+int
+Freeze::ConnectionI::__getRef() const
+{
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    return _refCount;
+}
+
+int
+Freeze::ConnectionI::__getRefNoSync() const
+{
+    return _refCount;
+}
+
+
 Freeze::ConnectionI::~ConnectionI()
 {
     close();
@@ -88,7 +126,9 @@ Freeze::ConnectionI::ConnectionI(const SharedDbEnvPtr& dbEnv) :
     _envName(dbEnv->getEnvName()),
     _trace(_communicator->getProperties()->getPropertyAsInt("Freeze.Trace.Map")),
     _txTrace(_communicator->getProperties()->getPropertyAsInt("Freeze.Trace.Transaction")),
-    _deadlockWarning(_communicator->getProperties()->getPropertyAsInt("Freeze.Warn.Deadlocks") != 0)
+    _deadlockWarning(_communicator->getProperties()->getPropertyAsInt("Freeze.Warn.Deadlocks") != 0),
+    _refCountMutex(new SharedMutex),
+    _refCount(0)
 {
 }
 
diff --git a/cpp/src/Freeze/ConnectionI.h b/cpp/src/Freeze/ConnectionI.h
index aa4fb822089..38b85aa9d2b 100644
--- a/cpp/src/Freeze/ConnectionI.h
+++ b/cpp/src/Freeze/ConnectionI.h
@@ -21,6 +21,17 @@ namespace Freeze
 
 class MapHelperI;
 
+//
+// A mutex shared by a connection and all its transactions
+// (for refcounting thread-safety)
+//
+struct SharedMutex : public IceUtil::Shared
+{
+    IceUtil::Mutex mutex;
+};
+typedef IceUtil::Handle<SharedMutex> SharedMutexPtr;
+
+
 class ConnectionI : public Connection
 {
 public:
@@ -35,6 +46,13 @@ public:
 
     virtual std::string getName() const;
 
+    //
+    // Custom refcounting implementation
+    //
+    virtual void __incRef();
+    virtual void __decRef();
+    virtual int __getRef() const;
+ 
     virtual ~ConnectionI();
 
     ConnectionI(const SharedDbEnvPtr&);
@@ -65,6 +83,10 @@ public:
 
 private:
 
+    friend class TransactionI;
+
+    int __getRefNoSync() const;
+
     const Ice::CommunicatorPtr _communicator;
     SharedDbEnvPtr _dbEnv;
     const std::string _envName;
@@ -73,6 +95,8 @@ private:
     const Ice::Int _trace;
     const Ice::Int _txTrace;
     const bool _deadlockWarning;
+    SharedMutexPtr _refCountMutex;
+    int _refCount;
 };  
 typedef IceUtil::Handle<ConnectionI> ConnectionIPtr;
 
diff --git a/cpp/src/Freeze/TransactionI.cpp b/cpp/src/Freeze/TransactionI.cpp
index f7844748ed8..a11c0eb44c4 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 explicitly call rollback or commit.";
+                }
             }
             
             _txn->abort();
@@ -139,45 +161,63 @@ Freeze::TransactionI::rollback()
     }
 }
 
-Freeze::ConnectionPtr
-Freeze::TransactionI::getConnection() const
+void
+Freeze::TransactionI::__incRef()
 {
-    return _connection;
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    _refCount++;
 }
 
-//
-// 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)
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    if(--_refCount == 0)
     {
-        rb = true;
+        sync.release();
+        delete this;
     }
-    Shared::__decRef();
-    if(rb)
+    else if(_txn != 0 && _refCount == 1 && _connection->__getRefNoSync() == 1)
     {
-        rollback();
-        // After this the transaction is dead.
+        sync.release();
+        rollbackInternal(true);
     }
 }
 
+int
+Freeze::TransactionI::__getRef() const
+{
+    IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+    return _refCount;
+}
+
+int
+Freeze::TransactionI::__getRefNoSync() const
+{
+    return _refCount;
+}
+
 void
 Freeze::TransactionI::setPostCompletionCallback(const Freeze::PostCompletionCallbackPtr& cb)
 {
     _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()),
-    _txn(0)
+    _warnRollback(_communicator->getProperties()->getPropertyAsIntWithDefault("Freeze.Warn.Rollback", 1)),
+    _txn(0),
+    _refCountMutex(connection->_refCountMutex),
+    _refCount(0)
 {
     try
     {
@@ -207,18 +247,7 @@ Freeze::TransactionI::TransactionI(const ConnectionIPtr& connection) :
     
 Freeze::TransactionI::~TransactionI()
 {
-    if(_txn != 0)
-    {
-        try
-        {
-            rollback();
-        }
-        catch(const IceUtil::Exception& e)
-        {
-            Error error(_communicator->getLogger());
-            error << "transaction rollback raised :" << e;
-        }
-    }
+    assert(_txn == 0);
 }
 
 void
@@ -228,26 +257,21 @@ Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
     // calling both the post completion callback and
     // Connection::clearTransaction may alter the transaction
     // reference count which checks _txn.
-    _txn = 0;
+
+    {
+        //
+        // We synchronize here as _txn is checked (read) in the refcounting code
+        //
+        IceUtil::Mutex::Lock sync(_refCountMutex->mutex);
+        _txn = 0;
+    }
 
     if(_postCompletionCallback != 0)
     {
         _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();
-    // At this point the transaction may be dead.
+    ConnectionIPtr connection = _connection;
+    _connection = 0;
+    connection->clearTransaction(); // may release the last _refCount
 }
diff --git a/cpp/src/Freeze/TransactionI.h b/cpp/src/Freeze/TransactionI.h
index a44e782bbc0..a46c9373969 100644
--- a/cpp/src/Freeze/TransactionI.h
+++ b/cpp/src/Freeze/TransactionI.h
@@ -20,6 +20,9 @@ namespace Freeze
 class ConnectionI;
 typedef IceUtil::Handle<ConnectionI> ConnectionIPtr;
 
+struct SharedMutex;
+typedef IceUtil::Handle<SharedMutex> SharedMutexPtr;
+
 class PostCompletionCallback : public virtual IceUtil::Shared
 {
 public:
@@ -36,16 +39,20 @@ public:
 
     virtual void rollback();
 
+
     virtual ConnectionPtr getConnection() const;
     
     //
     // Custom refcounting implementation
     //
+    virtual void __incRef();
     virtual void __decRef();
+    virtual int __getRef() const;
 
+    void rollbackInternal(bool);
     void setPostCompletionCallback(const PostCompletionCallbackPtr&);
 
-    TransactionI(const ConnectionIPtr&);
+    TransactionI(ConnectionI*);
     ~TransactionI();
     
     DbTxn*
@@ -58,13 +65,18 @@ private:
 
     friend class ConnectionI;
     
+    int __getRefNoSync() const;
+  
     void postCompletion(bool, bool);
 
     const Ice::CommunicatorPtr _communicator;
     ConnectionIPtr _connection;
     const Ice::Int _txTrace;
+    const Ice::Int _warnRollback;
     DbTxn* _txn;
     PostCompletionCallbackPtr _postCompletionCallback;
+    SharedMutexPtr _refCountMutex;
+    int _refCount;
 };
 
 typedef IceUtil::Handle<TransactionI> TransactionIPtr;
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)