IcePack bug fixes

6 files changed, 99 insertions, 62 deletions

diff --git a/cpp/CHANGES b/cpp/CHANGES
index f437c597f00..55378efd947 100644
--- a/cpp/CHANGES
+++ b/cpp/CHANGES
@@ -1,6 +1,9 @@
 Changes since version 1.5.1
 ---------------------------
 
+- Fixed bug in IcePack that would prevent dynamically added adapters
+  to be used again after the IcePack registry was restarted.
+
 - Fixed tracing of operation mode.
 
 - Fixed bug in slice2cpp: for statically linked binaries under
diff --git a/cpp/src/IcePack/AdapterRegistryI.cpp b/cpp/src/IcePack/AdapterRegistryI.cpp
index c0e43498a76..158c639a481 100644
--- a/cpp/src/IcePack/AdapterRegistryI.cpp
+++ b/cpp/src/IcePack/AdapterRegistryI.cpp
@@ -27,20 +27,39 @@ IcePack::AdapterRegistryI::AdapterRegistryI(const Ice::CommunicatorPtr& communic
 }
 
 void
-IcePack::AdapterRegistryI::add(const string& id, const AdapterPrx& adapter, const Ice::Current&)
+IcePack::AdapterRegistryI::add(const string& id, const AdapterPrx& adapter, const Ice::Current& current)
 {
-    Freeze::ConnectionPtr connection = Freeze::createConnection(_communicator, _envName);
-    StringObjectProxyDict dict(connection, _dbName); 
-
-    StringObjectProxyDict::iterator p = dict.find(id);
-    if(p != dict.end())
+    while(true)
     {
+	AdapterPrx oldAdapter;
 	try
 	{
-	    p->second->ice_ping();
+	    oldAdapter = findById(id, current);
+	    oldAdapter->ice_ping();
+	    throw AdapterExistsException();
+	}
+	catch(const AdapterNotExistException&)
+	{
 	}
 	catch(const Ice::ObjectNotExistException&)
 	{
+	}
+	catch(const Ice::LocalException&)
+	{
+	    throw AdapterExistsException();
+	}
+	
+	Freeze::ConnectionPtr connection = Freeze::createConnection(_communicator, _envName);
+	StringObjectProxyDict dict(connection, _dbName); 
+
+	StringObjectProxyDict::iterator p = dict.find(id);
+	if(p != dict.end())
+	{
+	    if(oldAdapter && oldAdapter != p->second)
+	    {
+		continue;
+	    }
+
 	    p.set(adapter);
 
 	    if(_traceLevels->adapterRegistry > 0)
@@ -48,20 +67,19 @@ IcePack::AdapterRegistryI::add(const string& id, const AdapterPrx& adapter, cons
 		Ice::Trace out(_traceLevels->logger, _traceLevels->adapterRegistryCat);
 		out << "added adapter `" << id << "'";
 	    }
-
-	    return;
 	}
-	catch(const Ice::LocalException&)
+	else
 	{
+	    dict.put(pair<const string, const Ice::ObjectPrx>(id, adapter));
+	    
+	    if(_traceLevels->adapterRegistry > 0)
+	    {
+		Ice::Trace out(_traceLevels->logger, _traceLevels->adapterRegistryCat);
+		out << "added adapter `" << id << "'";
+	    }
 	}
-	throw AdapterExistsException();
-    }
-    dict.put(pair<const string, const Ice::ObjectPrx>(id, adapter));
 
-    if(_traceLevels->adapterRegistry > 0)
-    {
-	Ice::Trace out(_traceLevels->logger, _traceLevels->adapterRegistryCat);
-	out << "added adapter `" << id << "'";
+	break;
     }
 }
 
diff --git a/cpp/src/IcePack/LocatorRegistryI.cpp b/cpp/src/IcePack/LocatorRegistryI.cpp
index 90eb357b43b..f28e45f4557 100644
--- a/cpp/src/IcePack/LocatorRegistryI.cpp
+++ b/cpp/src/IcePack/LocatorRegistryI.cpp
@@ -9,17 +9,19 @@
 
 #include <Ice/Ice.h>
 #include <IcePack/LocatorRegistryI.h>
-#include <IcePack/AdapterI.h>
+#include <IcePack/AdapterFactory.h>
 
 using namespace std;
 using namespace IcePack;
 
 IcePack::LocatorRegistryI::LocatorRegistryI(const AdapterRegistryPtr& adapterRegistry, 
                                             const ServerRegistryPtr& serverRegistry,
-					    const Ice::ObjectAdapterPtr& adapter) :
+					    const AdapterFactoryPtr& adapterFactory,
+					    bool dynamicRegistration) :
     _adapterRegistry(adapterRegistry),
     _serverRegistry(serverRegistry),
-    _adapter(adapter)
+    _adapterFactory(adapterFactory),
+    _dynamicRegistration(dynamicRegistration)
 {
 }
 
@@ -62,34 +64,27 @@ IcePack::LocatorRegistryI::setAdapterDirectProxy(const string& id, const Ice::Ob
 	}
 
 	//
-	// TODO: Review this functionnality.
+	// Create a new standalone adapter. The adapter will be
+	// persistent. It's the responsability of the user to cleanup
+	// adapter entries which were dynamically added from the
+	// registry.
 	//
-	// Create a new standalone adapter. This adapter will be
-	// destroyed when the registry is shutdown. Since it's not
-	// persistent, it won't be restored when the registry startup
-	// again. We could change this to make the adapter persistent
-	// but then it's not clear when this adapter should be
-	// destroyed.
-	//
-	// Also, is this really usefull? This allows a server (not
-	// deployed or managed by an IcePack node) to use the location
-	// mechanism (and avoid to share endpoints configuration
-	// between the client and server). Maybe it would be less
-	// confusing to just prevent servers to start if the server
-	// didn't previously registered its object adapters (using the
-	// IcePack deployment mechanism).
-	//
-	Ice::PropertiesPtr properties = _adapter->getCommunicator()->getProperties();
-	if(properties->getPropertyAsInt("IcePack.Registry.DynamicRegistration") > 0)
+	if(_dynamicRegistration)
 	{
-	    AdapterPrx adapter = AdapterPrx::uncheckedCast(_adapter->addWithUUID(new StandaloneAdapterI()));
 	    try
 	    {
-		_adapterRegistry->add(id, adapter);
+		AdapterPrx adapter = _adapterFactory->createStandaloneAdapter(id);
+		try
+		{
+		    _adapterRegistry->add(id, adapter);
+		}
+		catch(const AdapterExistsException&)
+		{
+		    adapter->destroy();
+		}
 	    }
-	    catch(const AdapterExistsException&)
+	    catch(const Ice::AlreadyRegisteredException&)
 	    {
-		_adapter->remove(adapter->ice_getIdentity());
 	    }
 	}
 	else
@@ -99,7 +94,7 @@ IcePack::LocatorRegistryI::setAdapterDirectProxy(const string& id, const Ice::Ob
     }
 }
 
-void 
+void
 IcePack::LocatorRegistryI::setServerProcessProxy(const string& name, const Ice::ProcessPrx& proxy, const Ice::Current&)
 {
     try
diff --git a/cpp/src/IcePack/LocatorRegistryI.h b/cpp/src/IcePack/LocatorRegistryI.h
index d512642f3d0..f76a1014ca8 100644
--- a/cpp/src/IcePack/LocatorRegistryI.h
+++ b/cpp/src/IcePack/LocatorRegistryI.h
@@ -17,11 +17,14 @@
 namespace IcePack
 {
 
+class AdapterFactory;
+typedef IceUtil::Handle<AdapterFactory> AdapterFactoryPtr;
+
 class LocatorRegistryI : public Ice::LocatorRegistry
 {
 public:
 
-    LocatorRegistryI(const AdapterRegistryPtr&, const ServerRegistryPtr&, const Ice::ObjectAdapterPtr&);
+    LocatorRegistryI(const AdapterRegistryPtr&, const ServerRegistryPtr&, const AdapterFactoryPtr&, bool);
     
     virtual void setAdapterDirectProxy(const ::std::string&, const ::Ice::ObjectPrx&, const ::Ice::Current&);
     virtual void setServerProcessProxy(const ::std::string&, const ::Ice::ProcessPrx&, const ::Ice::Current&);
@@ -30,7 +33,8 @@ private:
     
     AdapterRegistryPtr _adapterRegistry;
     ServerRegistryPtr _serverRegistry;
-    Ice::ObjectAdapterPtr _adapter;
+    AdapterFactoryPtr _adapterFactory;
+    const bool _dynamicRegistration;
 };
 
 }
diff --git a/cpp/src/IcePack/Registry.cpp b/cpp/src/IcePack/Registry.cpp
index 712d40d1527..4b4bca341d0 100644
--- a/cpp/src/IcePack/Registry.cpp
+++ b/cpp/src/IcePack/Registry.cpp
@@ -170,7 +170,8 @@ IcePack::Registry::start(bool nowarn, bool requiresInternalEndpoints)
     locatorRegistryId.category = "IcePack";
     locatorRegistryId.name = IceUtil::generateUUID();
     
-    ObjectPtr locatorRegistry = new LocatorRegistryI(adapterRegistry, serverRegistry, serverAdapter);
+    bool dynamicReg = properties->getPropertyAsInt("IcePack.Registry.DynamicRegistration") > 0;
+    ObjectPtr locatorRegistry = new LocatorRegistryI(adapterRegistry, serverRegistry, adapterFactory, dynamicReg);
     LocatorRegistryPrx locatorRegistryPrx = LocatorRegistryPrx::uncheckedCast(
 	serverAdapter->add(locatorRegistry, locatorRegistryId));
 
diff --git a/cpp/src/IcePack/ServerRegistryI.cpp b/cpp/src/IcePack/ServerRegistryI.cpp
index 42f637841dd..8f06a40fde9 100644
--- a/cpp/src/IcePack/ServerRegistryI.cpp
+++ b/cpp/src/IcePack/ServerRegistryI.cpp
@@ -34,35 +34,51 @@ void
 IcePack::ServerRegistryI::add(const string& name, 
 			      const ServerPrx& server,
 			      const ServerDescriptorPtr& descriptor,
-			      const Ice::Current&)
+			      const Ice::Current& current)
 {
-    Freeze::ConnectionPtr connection = Freeze::createConnection(_communicator, _envName);
-    StringObjectProxyDict dict(connection, _dbName); 
-    StringServerDescriptorDict dictDescriptor(connection, _dbDescriptorName);
-
-    StringObjectProxyDict::iterator p = dict.find(name);
-    if(p != dict.end())
+    while(true)
     {
+	ServerPrx oldServer;
 	try
 	{
-	    p->second->ice_ping();
+	    oldServer = findByName(name, current);
+	    oldServer->ice_ping();
 	    throw ServerExistsException();
 	}
+	catch(const ServerNotExistException&)
+	{
+	}
 	catch(const Ice::ObjectNotExistException&)
 	{
 	}
 	catch(const Ice::LocalException&)
 	{
+	    throw ServerExistsException();
+	}
+	
+	Freeze::ConnectionPtr connection = Freeze::createConnection(_communicator, _envName);
+	StringObjectProxyDict dict(connection, _dbName); 
+	StringServerDescriptorDict dictDescriptor(connection, _dbDescriptorName);
+	
+	StringObjectProxyDict::iterator p = dict.find(name);
+	if(p != dict.end())
+	{
+	    if(oldServer && oldServer != p->second)
+	    {
+		continue;
+	    }
 	}
-    }
-    
-    dict.put(pair<const string, const Ice::ObjectPrx>(name, server));
-    dictDescriptor.put(pair<const string, const ServerDescriptorPtr>(name, descriptor));
 
-    if(_traceLevels->serverRegistry > 0)
-    {
-	Ice::Trace out(_traceLevels->logger, _traceLevels->serverRegistryCat);
-	out << "added server `" << name << "'";
+	dict.put(pair<const string, const Ice::ObjectPrx>(name, server));
+	dictDescriptor.put(pair<const string, const ServerDescriptorPtr>(name, descriptor));
+	
+	if(_traceLevels->serverRegistry > 0)
+	{
+	    Ice::Trace out(_traceLevels->logger, _traceLevels->serverRegistryCat);
+	    out << "added server `" << name << "'";
+	}
+
+	break;
     }
 }