remove now returns the servant

11 files changed, 101 insertions, 74 deletions

diff --git a/cpp/slice/Freeze/Evictor.ice b/cpp/slice/Freeze/Evictor.ice
index 129dbcb9ab3..4b727b2d547 100644
--- a/cpp/slice/Freeze/Evictor.ice
+++ b/cpp/slice/Freeze/Evictor.ice
@@ -239,6 +239,8 @@ local interface Evictor extends Ice::ServantLocator
      *
      * @param id The identity of the &Ice; object.
      *
+     * @return The removed servant.
+     *
      * @throws NotRegisteredException Raised if this identity was not 
      * registered with the evictor.
      *
@@ -251,8 +253,7 @@ local interface Evictor extends Ice::ServantLocator
      * @see removeFacet
      *
      **/
-    void remove(Ice::Identity id);
-
+    Object remove(Ice::Identity id);
 
     /**
      *
@@ -264,6 +265,8 @@ local interface Evictor extends Ice::ServantLocator
      * @param facet The facet. An empty facet means the default
      * facet.
      *
+     * @return The removed servant.
+     *
      * @throws NotRegisteredException Raised if this identity was not 
      * registered with the evictor.
      *
@@ -277,7 +280,7 @@ local interface Evictor extends Ice::ServantLocator
      * @see addFacet
      *
      **/
-    void removeFacet(Ice::Identity id, string facet);
+    Object removeFacet(Ice::Identity id, string facet);
 
     /**
      *
diff --git a/cpp/slice/Ice/ObjectAdapter.ice b/cpp/slice/Ice/ObjectAdapter.ice
index 39b3d254475..d60532e88d3 100644
--- a/cpp/slice/Ice/ObjectAdapter.ice
+++ b/cpp/slice/Ice/ObjectAdapter.ice
@@ -241,12 +241,14 @@ local interface ObjectAdapter
      * objects. Removing an identity that is not in the map throws
      * [NotRegisteredException].
      *
+     * @return The removed servant.
+     *
      * @see Identity
      * @see add
      * @see addWithUUID
      *
      **/
-    void remove(Identity id);
+    Object remove(Identity id);
 
     /**
      *
@@ -260,12 +262,14 @@ local interface ObjectAdapter
      * @param facet The facet. An empty facet means the default
      * facet.
      *
+     * @return The removed servant.
+     *
      * @see Identity
      * @see addFacet
      * @see addFacetWithUUID
      *
      **/
-    void removeFacet(Identity id, string facet);
+    Object removeFacet(Identity id, string facet);
 
     /**
      *
diff --git a/cpp/src/Freeze/EvictorI.cpp b/cpp/src/Freeze/EvictorI.cpp
index 913af86a0ae..7ea78e088a2 100644
--- a/cpp/src/Freeze/EvictorI.cpp
+++ b/cpp/src/Freeze/EvictorI.cpp
@@ -734,20 +734,20 @@ Freeze::EvictorI::createObject(const Identity& ident, const ObjectPtr& servant)
     }
 }
 
-void
+Ice::ObjectPtr
 Freeze::EvictorI::remove(const Identity& ident)
 {
-    removeFacet(ident, "");
+    return removeFacet(ident, "");
 }
 
-void
+Ice::ObjectPtr
 Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 {
     checkIdentity(ident);
     DeactivateController::Guard deactivateGuard(_deactivateController);
    
     ObjectStore* store = findStore(facet);
-    bool notThere = (store == 0);
+    ObjectPtr servant = 0;
 
     if(store != 0)
     {
@@ -758,11 +758,7 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 	    //
 	    
 	    EvictorElementPtr element = store->pin(ident);
-	    if(element == 0)
-	    {
-		notThere = true;
-	    }
-	    else
+	    if(element != 0)
 	    {
 		Lock sync(*this);
 		if(element->stale)
@@ -781,6 +777,7 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 		    {
 			case EvictorElement::clean:
 			{
+			    servant = element->rec.servant;
 			    element->status = EvictorElement::destroyed;
 			    element->rec.servant = 0;
 			    addToModifiedQueue(element);
@@ -788,12 +785,14 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 			}
 			case EvictorElement::created:
 			{
+			    servant = element->rec.servant;
 			    element->status = EvictorElement::dead;
 			    element->rec.servant = 0;
 			    break;
 			}
 			case EvictorElement::modified:
 			{
+			    servant = element->rec.servant;
 			    element->status = EvictorElement::destroyed;
 			    element->rec.servant = 0;
 			    //
@@ -806,7 +805,6 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 			case EvictorElement::destroyed:
 			case EvictorElement::dead:
 			{
-			    notThere = true;
 			    break;
 			}
 			default:
@@ -818,7 +816,7 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 		}
 		if(element->keepCount > 0)
 		{
-		    assert(notThere == false);
+		    assert(servant != 0);
 
 		    element->keepCount = 0;
 		    //
@@ -835,7 +833,7 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 	}
     }
     
-    if(notThere)
+    if(servant == 0)
     {
 	NotRegisteredException ex(__FILE__, __LINE__);
 	ex.kindOfObject = "servant";
@@ -856,6 +854,7 @@ Freeze::EvictorI::removeFacet(const Identity& ident, const string& facet)
 	    out << " with facet \"" << facet << "\"";
 	}
     }
+    return servant;
 }
 
 //
diff --git a/cpp/src/Freeze/EvictorI.h b/cpp/src/Freeze/EvictorI.h
index 906dd672ed7..7e34c4b807b 100644
--- a/cpp/src/Freeze/EvictorI.h
+++ b/cpp/src/Freeze/EvictorI.h
@@ -125,8 +125,8 @@ public:
     virtual Ice::ObjectPrx addFacet(const Ice::ObjectPtr&, const Ice::Identity&, const std::string&);
     virtual void createObject(const Ice::Identity&, const Ice::ObjectPtr&);
 
-    virtual void remove(const Ice::Identity&);
-    virtual void removeFacet(const Ice::Identity&, const std::string&);
+    virtual Ice::ObjectPtr remove(const Ice::Identity&);
+    virtual Ice::ObjectPtr removeFacet(const Ice::Identity&, const std::string&);
     virtual void destroyObject(const Ice::Identity&);
 
     virtual void keep(const Ice::Identity&);
diff --git a/cpp/src/Ice/ObjectAdapterI.cpp b/cpp/src/Ice/ObjectAdapterI.cpp
index 88c175a885b..a3966c6870d 100644
--- a/cpp/src/Ice/ObjectAdapterI.cpp
+++ b/cpp/src/Ice/ObjectAdapterI.cpp
@@ -333,13 +333,13 @@ Ice::ObjectAdapterI::addFacetWithUUID(const ObjectPtr& object, const string& fac
     return addFacet(object, ident, facet);
 }
 
-void
+ObjectPtr
 Ice::ObjectAdapterI::remove(const Identity& ident)
 {
-    removeFacet(ident, "");
+    return removeFacet(ident, "");
 }
 
-void
+ObjectPtr
 Ice::ObjectAdapterI::removeFacet(const Identity& ident, const string& facet)
 {
     IceUtil::Monitor<IceUtil::RecMutex>::Lock sync(*this);
@@ -347,7 +347,7 @@ Ice::ObjectAdapterI::removeFacet(const Identity& ident, const string& facet)
     checkForDeactivation();
     checkIdentity(ident);
 
-    _servantManager->removeServant(ident, facet);
+    return _servantManager->removeServant(ident, facet);
 }
 
 FacetMap
diff --git a/cpp/src/Ice/ObjectAdapterI.h b/cpp/src/Ice/ObjectAdapterI.h
index f6e9819c213..03f6d7b3bb4 100644
--- a/cpp/src/Ice/ObjectAdapterI.h
+++ b/cpp/src/Ice/ObjectAdapterI.h
@@ -52,8 +52,8 @@ public:
     virtual ObjectPrx addFacet(const ObjectPtr&, const Identity&, const std::string&);
     virtual ObjectPrx addWithUUID(const ObjectPtr&);
     virtual ObjectPrx addFacetWithUUID(const ObjectPtr&, const std::string&);
-    virtual void remove(const Identity&);
-    virtual void removeFacet(const Identity&, const std::string&);
+    virtual ObjectPtr remove(const Identity&);
+    virtual ObjectPtr removeFacet(const Identity&, const std::string&);
     virtual FacetMap removeAllFacets(const Identity&);
     virtual ObjectPtr find(const Identity&);
     virtual ObjectPtr findFacet(const Identity&, const std::string&);
diff --git a/cpp/src/Ice/ServantManager.cpp b/cpp/src/Ice/ServantManager.cpp
index 0f926568a7e..4030dd8bf31 100644
--- a/cpp/src/Ice/ServantManager.cpp
+++ b/cpp/src/Ice/ServantManager.cpp
@@ -60,9 +60,16 @@ IceInternal::ServantManager::addServant(const ObjectPtr& object, const Identity&
     p->second.insert(pair<const string, ObjectPtr>(facet, object));
 }
 
-void
+ObjectPtr
 IceInternal::ServantManager::removeServant(const Identity& ident, const string& facet)
 {
+    //
+    // We return the removed servant to avoid releasing the last reference count
+    // with *this locked. We don't want to run user code, such as the servant
+    // destructor, with an internal Ice mutex locked.
+    //
+    ObjectPtr servant = 0;
+
     IceUtil::Mutex::Lock sync(*this);
     
     assert(_instance); // Must not be called after destruction.
@@ -87,6 +94,7 @@ IceInternal::ServantManager::removeServant(const Identity& ident, const string&
 	throw ex;
     }
 
+    servant = q->second;
     p->second.erase(q);
 
     if(p->second.empty())
@@ -101,6 +109,7 @@ IceInternal::ServantManager::removeServant(const Identity& ident, const string&
 	    _servantMapMap.erase(p);
 	}
     }
+    return servant;
 }
 
 FacetMap
@@ -297,38 +306,51 @@ IceInternal::ServantManager::~ServantManager()
 void
 IceInternal::ServantManager::destroy()
 {
-    IceUtil::Mutex::Lock sync(*this);
-    
-    assert(_instance); // Must not be called after destruction.
-
-    _servantMapMap.clear();
-    _servantMapMapHint = _servantMapMap.end();
+    ServantMapMap servantMapMap;
+    map<string, ServantLocatorPtr> locatorMap;
 
-    for(map<string, ServantLocatorPtr>::const_iterator p = _locatorMap.begin(); p != _locatorMap.end(); ++p)
     {
-	try
-	{
-	    p->second->deactivate(p->first);
-	}
-	catch(const Exception& ex)
+	IceUtil::Mutex::Lock sync(*this);
+	
+	assert(_instance); // Must not be called after destruction.
+	
+	servantMapMap.swap(_servantMapMap);
+	_servantMapMapHint = _servantMapMap.end();
+	
+	for(map<string, ServantLocatorPtr>::const_iterator p = _locatorMap.begin(); p != _locatorMap.end(); ++p)
 	{
-	    Error out(_instance->logger());
-	    out << "exception during locator deactivation:\n"
-		<< "object adapter: `" << _adapterName << "'\n"
-		<< "locator category: `" << p->first << "'\n"
-		<< ex;
-	}
-	catch(...)
-	{
-	    Error out(_instance->logger());
-	    out << "unknown exception during locator deactivation:\n"
-		<< "object adapter: `" << _adapterName << "'\n"
-		<< "locator category: `" << p->first << "'";
+	    try
+	    {
+		p->second->deactivate(p->first);
+	    }
+	    catch(const Exception& ex)
+	    {
+		Error out(_instance->logger());
+		out << "exception during locator deactivation:\n"
+		    << "object adapter: `" << _adapterName << "'\n"
+		    << "locator category: `" << p->first << "'\n"
+		    << ex;
+	    }
+	    catch(...)
+	    {
+		Error out(_instance->logger());
+		out << "unknown exception during locator deactivation:\n"
+		    << "object adapter: `" << _adapterName << "'\n"
+		    << "locator category: `" << p->first << "'";
+	    }
 	}
+	
+	locatorMap.swap(_locatorMap);
+	_locatorMapHint = _locatorMap.end();
+	
+	_instance = 0;
     }
 
-    _locatorMap.clear();
-    _locatorMapHint = _locatorMap.end();
-
-    _instance = 0;
+    //
+    // We clear the maps outside the synchronization as we don't want to
+    // hold any internal Ice mutex while running user code (such as servant
+    // or servant locator destructors). 
+    //
+    servantMapMap.clear();
+    locatorMap.clear();
 }
diff --git a/cpp/src/Ice/ServantManager.h b/cpp/src/Ice/ServantManager.h
index 9de57a7a219..3c107a5d3a4 100644
--- a/cpp/src/Ice/ServantManager.h
+++ b/cpp/src/Ice/ServantManager.h
@@ -33,7 +33,7 @@ class ServantManager : public IceUtil::Shared, public IceUtil::Mutex
 public:
 
     void addServant(const Ice::ObjectPtr&, const Ice::Identity&, const std::string&);
-    void removeServant(const Ice::Identity&, const std::string&);
+    Ice::ObjectPtr removeServant(const Ice::Identity&, const std::string&);
     Ice::FacetMap removeAllFacets(const Ice::Identity&);
     Ice::ObjectPtr findServant(const Ice::Identity&, const std::string&) const;
     Ice::FacetMap findAllFacets(const Ice::Identity&) const;
diff --git a/java/src/Freeze/EvictorI.java b/java/src/Freeze/EvictorI.java
index dfb769c98a3..fc995e3a9b5 100644
--- a/java/src/Freeze/EvictorI.java
+++ b/java/src/Freeze/EvictorI.java
@@ -720,13 +720,13 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
     }
 
     
-    public void
+    public Ice.Object
     remove(Ice.Identity ident)
     {
-	removeFacet(ident, "");
+	return removeFacet(ident, "");
     }
 
-    public void
+    public Ice.Object
     removeFacet(Ice.Identity ident, String facet)
     {
 	checkIdentity(ident);
@@ -752,7 +752,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 	try
 	{    
 	    ObjectStore store = findStore(facet);
-	    boolean notThere = (store == null);
+	    Ice.Object servant = null;
 	    
 	    if(store != null)
 	    {
@@ -763,11 +763,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 		    //
 		    
 		    EvictorElement element = (EvictorElement) store.cache().pin(ident);
-		    if(element == null)
-		    {
-			notThere = true;
-		    }
-		    else
+		    if(element != null)
 		    {
 			synchronized(this)
 			{
@@ -786,6 +782,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 				{
 				    case EvictorElement.clean:
 				    {
+					servant = element.rec.servant;
 					element.status = EvictorElement.destroyed;
 					element.rec.servant = null;
 					addToModifiedQueue(element);
@@ -793,12 +790,14 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 				    }
 				    case EvictorElement.created:
 				    {
+					servant = element.rec.servant;
 					element.status = EvictorElement.dead;
 					element.rec.servant = null;
 					break;
 				    }
 				    case EvictorElement.modified:
 				    {
+					servant = element.rec.servant;
 					element.status = EvictorElement.destroyed;
 					element.rec.servant = null;
 					//
@@ -811,7 +810,6 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 				    case EvictorElement.destroyed:
 				    case EvictorElement.dead:
 				    {
-					notThere = true;
 					break;
 				    }
 				    default:
@@ -824,7 +822,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 			    
 			    if(element. keepCount > 0)
 			    {
-				assert notThere == false;
+				assert servant != null;
 				
 				element.keepCount = 0;
 				//
@@ -846,7 +844,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 		}
 	    }
 	    
-	    if(notThere)
+	    if(servant == null)
 	    {
 		Ice.NotRegisteredException ex = new Ice.NotRegisteredException();
 		ex.kindOfObject = "servant";
@@ -870,6 +868,7 @@ class EvictorI extends Ice.LocalObjectImpl implements Evictor, Runnable
 		    "Freeze.Evictor", 
 		    "removed " + objString);
 	    }
+	    return servant;
 	}
 	finally
 	{
diff --git a/java/src/Ice/ObjectAdapterI.java b/java/src/Ice/ObjectAdapterI.java
index 6d6512cc201..84ba086ed1c 100644
--- a/java/src/Ice/ObjectAdapterI.java
+++ b/java/src/Ice/ObjectAdapterI.java
@@ -325,19 +325,19 @@ public final class ObjectAdapterI extends LocalObjectImpl implements ObjectAdapt
         return addFacet(object, ident, facet);
     }
 
-    public void
+    public Ice.Object
     remove(Identity ident)
     {
-        removeFacet(ident, "");
+        return removeFacet(ident, "");
     }
 
-    public synchronized void
+    public synchronized Ice.Object
     removeFacet(Identity ident, String facet)
     {
 	checkForDeactivation();
         checkIdentity(ident);
 
-	_servantManager.removeServant(ident, facet);
+	return _servantManager.removeServant(ident, facet);
     }
 
     public synchronized java.util.Map
diff --git a/java/src/IceInternal/ServantManager.java b/java/src/IceInternal/ServantManager.java
index 77c674756d2..4e89eb4aa9f 100644
--- a/java/src/IceInternal/ServantManager.java
+++ b/java/src/IceInternal/ServantManager.java
@@ -45,7 +45,7 @@ public final class ServantManager extends Thread
         m.put(facet, servant);
     }
 
-    public synchronized void
+    public synchronized Ice.Object
     removeServant(Ice.Identity ident, String facet)
     {
 	assert(_instance != null); // Must not be called after destruction.
@@ -57,7 +57,7 @@ public final class ServantManager extends Thread
 
         java.util.HashMap m = (java.util.HashMap)_servantMapMap.get(ident);
         Ice.Object obj = null;
-        if(m == null || (obj = (Ice.Object)m.get(facet)) == null)
+        if(m == null || (obj = (Ice.Object)m.remove(facet)) == null)
 	{
 	    Ice.NotRegisteredException ex = new Ice.NotRegisteredException();
 	    ex.id = Ice.Util.identityToString(ident);
@@ -69,11 +69,11 @@ public final class ServantManager extends Thread
 	    throw ex;
 	}
 
-        m.remove(facet);
         if(m.isEmpty())
         {
             _servantMapMap.remove(ident);
         }
+	return obj;
     }
 
     public synchronized java.util.Map