Fixed bug #2233

7 files changed, 250 insertions, 152 deletions

diff --git a/cpp/include/IceUtil/Shared.h b/cpp/include/IceUtil/Shared.h
index c80b6b35c5b..61b466d9895 100644
--- a/cpp/include/IceUtil/Shared.h
+++ b/cpp/include/IceUtil/Shared.h
@@ -197,57 +197,10 @@ public:
         return *this;
     }
 
-    void __incRef()
-    {
-#if defined(_WIN32)
-        assert(InterlockedExchangeAdd(&_ref, 0) >= 0);
-        InterlockedIncrement(&_ref);
-#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)
-        assert(ice_atomic_exchange_add(0, &_ref) >= 0);
-        ice_atomic_inc(&_ref);
-#else
-        _mutex.lock();
-        assert(_ref >= 0);
-        ++_ref;
-        _mutex.unlock();
-#endif
-    }
-
-    void __decRef()
-    {
-#if defined(_WIN32)
-        assert(InterlockedExchangeAdd(&_ref, 0) > 0);
-        if(InterlockedDecrement(&_ref) == 0 && !_noDelete)
-        {
-            _noDelete = true;
-            delete this;
-        }
-#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)
-        assert(ice_atomic_exchange_add(0, &_ref) > 0);
-        if(ice_atomic_dec_and_test(&_ref) && !_noDelete)
-        {
-            _noDelete = true;
-            delete this;
-        }
-#else
-        _mutex.lock();
-        bool doDelete = false;
-        assert(_ref > 0);
-        if(--_ref == 0)
-        {
-            doDelete = !_noDelete;
-            _noDelete = true;
-        }
-        _mutex.unlock();
-        if(doDelete)
-        {
-            delete this;
-        }
-#endif
-    }
-
-    int __getRef() const;
-    void __setNoDelete(bool);
+    virtual void __incRef();
+    virtual void __decRef();
+    virtual int __getRef() const;
+    virtual void __setNoDelete(bool);
 
 protected:
 
diff --git a/cpp/src/Freeze/ConnectionI.cpp b/cpp/src/Freeze/ConnectionI.cpp
index ca661331c8e..1237a46c753 100644
--- a/cpp/src/Freeze/ConnectionI.cpp
+++ b/cpp/src/Freeze/ConnectionI.cpp
@@ -31,6 +31,7 @@ Freeze::ConnectionI::beginTransactionI()
     }
     closeAllIterators();
     _transaction = new TransactionI(this);
+    _transaction->internalIncRef();
     return _transaction;
 }
 
@@ -49,7 +50,7 @@ Freeze::ConnectionI::close()
         {
             _transaction->rollback();
         }
-        catch(const  DatabaseException&)
+        catch(const DatabaseException&)
         {
             //
             // Ignored
@@ -90,6 +91,7 @@ Freeze::ConnectionI::ConnectionI(const SharedDbEnvPtr& dbEnv) :
     _communicator(dbEnv->getCommunicator()),
     _dbEnv(dbEnv),
     _envName(dbEnv->getEnvName()),
+    _transaction(0),
     _trace(_communicator->getProperties()->getPropertyAsInt("Freeze.Trace.Map")),
     _txTrace(_communicator->getProperties()->getPropertyAsInt("Freeze.Trace.Transaction")),
     _deadlockWarning(_communicator->getProperties()->getPropertyAsInt("Freeze.Warn.Deadlocks") != 0)
diff --git a/cpp/src/Freeze/ConnectionI.h b/cpp/src/Freeze/ConnectionI.h
index da12f1bd4fb..a9011da45e8 100644
--- a/cpp/src/Freeze/ConnectionI.h
+++ b/cpp/src/Freeze/ConnectionI.h
@@ -68,7 +68,7 @@ private:
     Ice::CommunicatorPtr _communicator;
     SharedDbEnvPtr _dbEnv;
     std::string _envName;
-    TransactionIPtr _transaction;
+    TransactionI* _transaction;
     std::list<MapHelperI*> _mapList;
     Ice::Int _trace;
     Ice::Int _txTrace;
@@ -78,7 +78,9 @@ private:
 inline void
 ConnectionI::clearTransaction()
 {
+    TransactionI* tx = _transaction;
     _transaction = 0;
+    tx->internalDecRef();
 }
 
 inline DbTxn*
diff --git a/cpp/src/Freeze/TransactionI.cpp b/cpp/src/Freeze/TransactionI.cpp
index 7537818596e..e5b28a79132 100644
--- a/cpp/src/Freeze/TransactionI.cpp
+++ b/cpp/src/Freeze/TransactionI.cpp
@@ -82,58 +82,105 @@ Freeze::TransactionI::commit()
 void
 Freeze::TransactionI::rollback()
 {
-    assert(_txn != 0);
-
-    long txnId = 0;
-    try
+    if(_txn != 0)
     {
-        _connection->closeAllIterators();
-
-        if(_txTrace >= 1)
+        long txnId = 0;
+        try
         {
-            txnId = (_txn->id() & 0x7FFFFFFF) + 0x80000000L;
+            _connection->closeAllIterators();
+            
+            if(_txTrace >= 1)
+            {
+                txnId = (_txn->id() & 0x7FFFFFFF) + 0x80000000L;
+            }
+            
+            _txn->abort();
+            
+            if(_txTrace >= 1)
+            {
+                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                out << "rolled back transaction " << hex << txnId << dec;
+            }
         }
-
-        _txn->abort();
-
-        if(_txTrace >= 1)
+        catch(const ::DbDeadlockException& dx)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
-            out << "rolled back transaction " << hex << txnId << dec;
+            if(_txTrace >= 1)
+            {
+                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
+            }
+            
+            postCompletion(false, true);
+            throw DeadlockException(__FILE__, __LINE__, dx.what());
         }
-    }
-    catch(const ::DbDeadlockException& dx)
-    {
-        if(_txTrace >= 1)
+        catch(const ::DbException& dx)
         {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
-            out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
+            if(_txTrace >= 1)
+            {
+                Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
+                out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
+            }
+            
+            postCompletion(false, false);
+            throw DatabaseException(__FILE__, __LINE__, dx.what());
         }
+        postCompletion(true, false);
+    }
+}
 
-        postCompletion(false, true);
-        throw DeadlockException(__FILE__, __LINE__, dx.what());
+Freeze::ConnectionPtr
+Freeze::TransactionI::getConnection() const
+{
+    if(_txn != 0)
+    {
+        return _connection;
     }
-    catch(const ::DbException& dx)
+    else
     {
-        if(_txTrace >= 1)
-        {
-            Trace out(_connection->communicator()->getLogger(), "Freeze.Transaction");
-            out << "failed to rollback transaction " << hex << txnId << dec << ": " << dx.what();
-        }
+        return 0;
+    }
+}
 
-        postCompletion(false, false);
-        throw DatabaseException(__FILE__, __LINE__, dx.what());
+//
+// External refcount operations, from code holding a Transaction[I]Ptr
+//
+void
+Freeze::TransactionI::__incRef()
+{
+    Shared::__incRef();
+    if(_txn != 0)
+    {
+         _connection->__incRef();
     }
-    postCompletion(true, false);
 }
 
-Freeze::ConnectionPtr
-Freeze::TransactionI::getConnection() const
+void
+Freeze::TransactionI::__decRef()
+{
+    if(_txn != 0)
+    {
+         _connection->__decRef();
+    }
+    Shared::__decRef();
+}
+
+//
+// The refcount on transaction held by the associated ConnectionI object
+//
+void
+Freeze::TransactionI::internalIncRef()
 {
-    return _connection;
+    Shared::__incRef();
 }
 
 void
+Freeze::TransactionI::internalDecRef()
+{
+    Shared::__incRef();
+}
+
+
+void
 Freeze::TransactionI::setPostCompletionCallback(const Freeze::PostCompletionCallbackPtr& cb)
 {
     _postCompletionCallback = cb;
@@ -175,15 +222,21 @@ Freeze::TransactionI::~TransactionI()
 {
     if(_txn != 0)
     {
-        rollback();
+        try
+        {
+            rollback();
+        }
+        catch(const IceUtil::Exception& e)
+        {
+            Error error(_connection->communicator()->getLogger());
+            error << "transaction rollback raised :" << e;
+        }
     }
 }
 
 void
 Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
 {
-    ConnectionIPtr connection = _connection;
-    _connection = 0;
     _txn = 0;
 
     if(_postCompletionCallback != 0)
@@ -191,5 +244,5 @@ Freeze::TransactionI::postCompletion(bool committed, bool deadlock)
         _postCompletionCallback->postCompletion(committed, deadlock);
     }
 
-    connection->clearTransaction();
+    _connection->clearTransaction(); // will release the internal refcount
 }
diff --git a/cpp/src/Freeze/TransactionI.h b/cpp/src/Freeze/TransactionI.h
index 7a7922d1297..b8e5a9a582d 100644
--- a/cpp/src/Freeze/TransactionI.h
+++ b/cpp/src/Freeze/TransactionI.h
@@ -38,6 +38,12 @@ public:
 
     virtual ConnectionPtr getConnection() const;
     
+    //
+    // Custom refcounting implementation
+    //
+    virtual void __incRef();
+    virtual void __decRef();
+
     void setPostCompletionCallback(const PostCompletionCallbackPtr&);
 
     TransactionI(ConnectionI*);
@@ -49,17 +55,22 @@ public:
         return _txn;
     }
 
-    const ConnectionIPtr&
+    const ConnectionI*
     getConnectionI() const
     {
         return _connection;
     }
 
 private:
+
+    friend class ConnectionI;
+    
+    void internalIncRef();
+    void internalDecRef();
     
     void postCompletion(bool, bool);
 
-    ConnectionIPtr _connection;
+    ConnectionI* const _connection;
     Ice::Int _txTrace;
     DbTxn* _txn;
     PostCompletionCallbackPtr _postCompletionCallback;
diff --git a/cpp/src/IceUtil/Shared.cpp b/cpp/src/IceUtil/Shared.cpp
index a0fa6a59f69..48823e3f21e 100755
--- a/cpp/src/IceUtil/Shared.cpp
+++ b/cpp/src/IceUtil/Shared.cpp
@@ -6,62 +6,113 @@
 // ICE_LICENSE file included in this distribution.
 //
 // **********************************************************************
-

-#include <IceUtil/Shared.h>

-

-using namespace IceUtil;

-

-IceUtil::SimpleShared::SimpleShared() :

-    _ref(0),

-    _noDelete(false)

-{

-}

-

-IceUtil::SimpleShared::SimpleShared(const SimpleShared&) :

-    _ref(0),

-    _noDelete(false)

-{

-}

-

-IceUtil::Shared::Shared() :

-#ifndef ICE_HAS_ATOMIC_FUNCTIONS

-    _ref(0),

-#endif

-    _noDelete(false)

-{

-#ifdef ICE_HAS_ATOMIC_FUNCTIONS

-    ice_atomic_set(&_ref, 0);

-#endif

-}

-

-IceUtil::Shared::Shared(const Shared&) :

-#ifndef ICE_HAS_ATOMIC_FUNCTIONS

-    _ref(0),

-#endif

-    _noDelete(false)

-{

-#ifdef ICE_HAS_ATOMIC_FUNCTIONS

-    ice_atomic_set(&_ref, 0);

-#endif

-}

-

-int

-IceUtil::Shared::__getRef() const

-{

-#if defined(_WIN32)

-    return InterlockedExchangeAdd(const_cast<LONG*>(&_ref), 0);

-#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)

-    return ice_atomic_exchange_add(0, const_cast<ice_atomic_t*>(&_ref));

-#else

-    _mutex.lock();

-    int ref = _ref;

-    _mutex.unlock();

-    return ref;

-#endif

-}

-

-void

-IceUtil::Shared::__setNoDelete(bool b)

-{

-    _noDelete = b;

-}

+
+#include <IceUtil/Shared.h>
+
+using namespace IceUtil;
+
+IceUtil::SimpleShared::SimpleShared() :
+    _ref(0),
+    _noDelete(false)
+{
+}
+
+IceUtil::SimpleShared::SimpleShared(const SimpleShared&) :
+    _ref(0),
+    _noDelete(false)
+{
+}
+
+IceUtil::Shared::Shared() :
+#ifndef ICE_HAS_ATOMIC_FUNCTIONS
+    _ref(0),
+#endif
+    _noDelete(false)
+{
+#ifdef ICE_HAS_ATOMIC_FUNCTIONS
+    ice_atomic_set(&_ref, 0);
+#endif
+}
+
+IceUtil::Shared::Shared(const Shared&) :
+#ifndef ICE_HAS_ATOMIC_FUNCTIONS
+    _ref(0),
+#endif
+    _noDelete(false)
+{
+#ifdef ICE_HAS_ATOMIC_FUNCTIONS
+    ice_atomic_set(&_ref, 0);
+#endif
+}
+
+void 
+IceUtil::Shared::__incRef()
+{
+#if defined(_WIN32)
+    assert(InterlockedExchangeAdd(&_ref, 0) >= 0);
+    InterlockedIncrement(&_ref);
+#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)
+    assert(ice_atomic_exchange_add(0, &_ref) >= 0);
+    ice_atomic_inc(&_ref);
+#else
+    _mutex.lock();
+    assert(_ref >= 0);
+    ++_ref;
+    _mutex.unlock();
+#endif
+}
+
+void 
+IceUtil::Shared::__decRef()
+{
+#if defined(_WIN32)
+    assert(InterlockedExchangeAdd(&_ref, 0) > 0);
+    if(InterlockedDecrement(&_ref) == 0 && !_noDelete)
+    {
+        _noDelete = true;
+        delete this;
+    }
+#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)
+    assert(ice_atomic_exchange_add(0, &_ref) > 0);
+    if(ice_atomic_dec_and_test(&_ref) && !_noDelete)
+    {
+        _noDelete = true;
+        delete this;
+    }
+#else
+    _mutex.lock();
+    bool doDelete = false;
+    assert(_ref > 0);
+    if(--_ref == 0)
+    {
+        doDelete = !_noDelete;
+        _noDelete = true;
+    }
+    _mutex.unlock();
+    if(doDelete)
+    {
+        delete this;
+    }
+#endif
+}
+
+int
+IceUtil::Shared::__getRef() const
+{
+#if defined(_WIN32)
+    return InterlockedExchangeAdd(const_cast<LONG*>(&_ref), 0);
+#elif defined(ICE_HAS_ATOMIC_FUNCTIONS)
+    return ice_atomic_exchange_add(0, const_cast<ice_atomic_t*>(&_ref));
+#else
+    _mutex.lock();
+    int ref = _ref;
+    _mutex.unlock();
+    return ref;
+#endif
+}
+
+void
+IceUtil::Shared::__setNoDelete(bool b)
+{
+    _noDelete = b;
+}
diff --git a/cpp/test/Freeze/dbmap/Client.cpp b/cpp/test/Freeze/dbmap/Client.cpp
index aeefc3a2133..17af3c7120b 100644
--- a/cpp/test/Freeze/dbmap/Client.cpp
+++ b/cpp/test/Freeze/dbmap/Client.cpp
@@ -584,6 +584,31 @@ run(const CommunicatorPtr& communicator, const string& envName)
     test(count == 4);
     cout << "ok " << endl;
 
+    cout << "Testing unreferenced connection+transaction... " << flush;
+    {
+        Freeze::ConnectionPtr c2 = createConnection(communicator, envName);
+        ByteIntMap m2(c2, dbName);
+
+        TransactionPtr tx = c2->beginTransaction();
+
+        p = m2.findByValue(17);
+        test(p != m2.end());
+
+        m2.put(ByteIntMap::value_type(alphabet[21], static_cast<Int>(99)));
+
+        p = m2.findByValue(17);
+        test(p == m2.end());
+
+        test(c2->currentTransaction() != 0);
+        test(tx->getConnection() != 0);
+    }
+    //
+    // Should roll back here
+    //
+    p = m.findByValue(17);
+    test(p != m.end());
+    cout << "ok " << endl;
+
     cout << "testing concurrent access... " << flush;
     m.clear();
     populateDB(connection, m);
@@ -602,6 +627,7 @@ run(const CommunicatorPtr& communicator, const string& envName)
     }
     cout << "ok" << endl;
 
+
     cout << "testing index creation... " << flush;
     
     {