Fixed deadlock in Freeze transactional evictor

4 files changed, 45 insertions, 71 deletions

diff --git a/cpp/include/IceUtil/Cache.h b/cpp/include/IceUtil/Cache.h
index 04afc7564b7..1449734e908 100644
--- a/cpp/include/IceUtil/Cache.h
+++ b/cpp/include/IceUtil/Cache.h
@@ -57,12 +57,10 @@ public:
 
     typedef typename std::map<Key, CacheValue>::iterator Position;
 
-
-    Handle<Value> getIfPinned(const Key&) const;
+    Handle<Value> getIfPinned(const Key&, bool = false) const;
 
     void unpin(Position);
-    Handle<Value> unpin(const Key&);
-
+  
     void clear();
     size_t size() const;
 
@@ -95,62 +93,28 @@ private:
 
 
 template<typename Key, typename Value> Handle<Value> 
-Cache<Key, Value>::getIfPinned(const Key& key) const
-{
-    Mutex::Lock sync(_mutex);
-    typename CacheMap::const_iterator p = _map.find(key);
-    if(p != _map.end())
-    {
-        return (*p).second.obj;
-    }
-    else
-    {
-        return 0;
-    }
-}
-
-template<typename Key, typename Value> void
-Cache<Key, Value>::unpin(typename Cache::Position p)
-{
-    //
-    // There is no risk to erase a 'being loaded' position,
-    // since such position nevr got outside yet!
-    //
-    Mutex::Lock sync(_mutex);
-    _map.erase(p);
-}
-
-template<typename Key, typename Value> Handle<Value>
-Cache<Key, Value>::unpin(const Key& key)
+Cache<Key, Value>::getIfPinned(const Key& key, bool wait) const
 {
     Mutex::Lock sync(_mutex);
 
     for(;;)
     {
-        typename CacheMap::iterator p = _map.find(key);
-        
-        if(p == _map.end())
-        {
-            return 0;
-        }
-     
-        Handle<Value> result = p->second.obj;
-   
-        if(result != 0)
-        {
-            _map.erase(p);
-            return result;
-        }
-        else
+        typename CacheMap::const_iterator p = _map.find(key);
+        if(p != _map.end())
         {
+            Handle<Value> result = (*p).second.obj;
+            if(result != 0 || wait == false)
+            {
+                return result;
+            }
+            
             //
-            // The object is being loaded so we need to wait:
-            // we can't erase 'p' while the loading thread is using it
+            // The object is being loaded: we wait
             //
 
             if(p->second.latch == 0)
             {
-                p->second.latch = new Latch;
+                const_cast<CacheValue&>(p->second).latch = new Latch;
             }
             
             Latch* latch = p->second.latch;
@@ -166,9 +130,24 @@ Cache<Key, Value>::unpin(const Key& key)
             // Try again
             //
         }
+        else
+        {
+            return 0;
+        }
     }
 }
 
+template<typename Key, typename Value> void
+Cache<Key, Value>::unpin(typename Cache::Position p)
+{
+    //
+    // There is no risk to erase a 'being loaded' position,
+    // since such position nevr got outside yet!
+    //
+    Mutex::Lock sync(_mutex);
+    _map.erase(p);
+}
+
 template<typename Key, typename Value> void 
 Cache<Key, Value>::clear()
 {
diff --git a/cpp/src/Freeze/TransactionalEvictorI.cpp b/cpp/src/Freeze/TransactionalEvictorI.cpp
index 278f37bc027..273eb4c6216 100644
--- a/cpp/src/Freeze/TransactionalEvictorI.cpp
+++ b/cpp/src/Freeze/TransactionalEvictorI.cpp
@@ -628,7 +628,7 @@ Freeze::TransactionalEvictorI::evict()
         //
         // Evict, no matter what!
         //
-        evict(*_evictorList.rbegin(), true);
+        evict(*_evictorList.rbegin());
     }
 }
 
@@ -667,45 +667,39 @@ Freeze::TransactionalEvictorI::loadCachedServant(const Identity& ident, ObjectSt
 Ice::ObjectPtr
 Freeze::TransactionalEvictorI::evict(const Identity& ident, ObjectStore<TransactionalEvictorElement>* store)
 {
-    Lock sync(*this);
-    TransactionalEvictorElementPtr element = store->unpin(ident);
-
+    //
+    // Important: we can't wait for the DB (even indirectly) with 'this' locked
+    //
+    TransactionalEvictorElementPtr element = store->getIfPinned(ident, true);
+    
     if(element != 0)
     {
-        evict(element, false);
-        return element->servant();
+        Lock sync(*this);
+        if(!element->_stale)
+        {
+            evict(element);
+            return element->servant();
+        }
     }
     return 0;
 }
 
 void 
-Freeze::TransactionalEvictorI::evict(const TransactionalEvictorElementPtr& element, bool unpin)
+Freeze::TransactionalEvictorI::evict(const TransactionalEvictorElementPtr& element)
 {
     //
     // Must be called with this locked!
     //
     assert(!element->_stale);
     element->_stale = true;
-
-    if(unpin)
-    {
-        element->_store.unpin(element->_cachePosition);
-    }
-
+    element->_store.unpin(element->_cachePosition);
+  
     if(element->_inEvictor)
     {
         element->_inEvictor = false;
         _evictorList.erase(element->_evictPosition);
         _currentEvictorSize--;
     }
-    else
-    {
-        //
-        // This object was removed before it had time to make it into
-        // the evictor
-        //
-        assert(!unpin);
-    }
 }
 
 void 
diff --git a/cpp/src/Freeze/TransactionalEvictorI.h b/cpp/src/Freeze/TransactionalEvictorI.h
index a55700a98bf..caf1484447d 100644
--- a/cpp/src/Freeze/TransactionalEvictorI.h
+++ b/cpp/src/Freeze/TransactionalEvictorI.h
@@ -104,7 +104,7 @@ private:
 
     Ice::ObjectPtr loadCachedServant(const Ice::Identity&, ObjectStore<TransactionalEvictorElement>*);
 
-    void evict(const TransactionalEvictorElementPtr&, bool);
+    void evict(const TransactionalEvictorElementPtr&);
     void fixEvictPosition(const TransactionalEvictorElementPtr&);
 
     void servantNotFound(const char*, int, const Ice::Current&);
diff --git a/cpp/test/Freeze/evictor/config b/cpp/test/Freeze/evictor/config
index 29c8730d6bd..845603239f8 100644
--- a/cpp/test/Freeze/evictor/config
+++ b/cpp/test/Freeze/evictor/config
@@ -9,3 +9,4 @@ Ice.ThreadPool.Server.SizeWarn=0
 
 Freeze.Evictor.db.Test.RollbackOnUserException=1
 
+#Freeze.Warn.Deadlocks=1