http://bugzilla.zeroc.com/bugzilla/show_bug.cgi?id=448

4 files changed, 97 insertions, 189 deletions

diff --git a/cpp/src/Ice/CommunicatorI.cpp b/cpp/src/Ice/CommunicatorI.cpp
index de757ed2cde..47092b0adda 100644
--- a/cpp/src/Ice/CommunicatorI.cpp
+++ b/cpp/src/Ice/CommunicatorI.cpp
@@ -71,19 +71,7 @@ printGCStats(const IceInternal::GCStats& stats)
 void
 Ice::CommunicatorI::destroy()
 {
-    InstancePtr instance;
-
-    {
-	RecMutex::Lock sync(*this);
-	
-	if(_destroyed) // Don't destroy twice.
-	{
-	    return;
-	}
-	_destroyed = true;
-	instance = _instance;
-    }
-
+    if(_instance->destroy())
     {
 	IceUtil::StaticMutex::Lock sync(gcMutex);
 
@@ -117,91 +105,36 @@ Ice::CommunicatorI::destroy()
 	    theCollector = 0; // Force destruction of the collector.
 	}
     }
-
-    if(instance)
-    {
-	instance->destroy();
-    }
 }
 
 void
 Ice::CommunicatorI::shutdown()
 { 
-    ObjectAdapterFactoryPtr objectAdapterFactory;
-
-    {
-	RecMutex::Lock sync(*this);
-	if(_destroyed)
-	{
-	    throw CommunicatorDestroyedException(__FILE__, __LINE__);
-	}
-	objectAdapterFactory = _instance->objectAdapterFactory();
-    }
-
-    //
-    // We must call shutdown on the object adapter factory outside the
-    // synchronization, otherwise the communicator is blocked during
-    // shutdown.
-    //
-    objectAdapterFactory->shutdown();
+    _instance->objectAdapterFactory()->shutdown();
 }
 
 void
 Ice::CommunicatorI::waitForShutdown()
 {
-    ObjectAdapterFactoryPtr objectAdapterFactory;
-
-    {
-	RecMutex::Lock sync(*this);
-	if(_destroyed)
-	{
-	    throw CommunicatorDestroyedException(__FILE__, __LINE__);
-	}
-	objectAdapterFactory = _instance->objectAdapterFactory();
-    }
-
-    //
-    // We must call waitForShutdown on the object adapter factory
-    // outside the synchronization, otherwise the communicator is
-    // blocked while we wait for shutdown.
-    //
-    objectAdapterFactory->waitForShutdown();
+    _instance->objectAdapterFactory()->waitForShutdown();
 }
 
 ObjectPrx
 Ice::CommunicatorI::stringToProxy(const string& s) const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->proxyFactory()->stringToProxy(s);
 }
 
 string
 Ice::CommunicatorI::proxyToString(const ObjectPrx& proxy) const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->proxyFactory()->proxyToString(proxy);
 }
 
 ObjectAdapterPtr
 Ice::CommunicatorI::createObjectAdapter(const string& name)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
-    
-    ObjectAdapterPtr adapter = _instance->objectAdapterFactory()->createObjectAdapter(name);
-
-    return adapter;
+    return _instance->objectAdapterFactory()->createObjectAdapter(name);
 }
 
 ObjectAdapterPtr
@@ -227,163 +160,90 @@ Ice::CommunicatorI::createObjectAdapterWithEndpoints(const string& name, const s
 void
 Ice::CommunicatorI::addObjectFactory(const ObjectFactoryPtr& factory, const string& id)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->servantFactoryManager()->add(factory, id);
 }
 
 void
 Ice::CommunicatorI::removeObjectFactory(const string& id)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->servantFactoryManager()->remove(id);
 }
 
 ObjectFactoryPtr
 Ice::CommunicatorI::findObjectFactory(const string& id) const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->servantFactoryManager()->find(id);
 }
 
 PropertiesPtr
 Ice::CommunicatorI::getProperties() const
 {
-    //
-    // No check for destruction. It must be possible to access the
-    // properties after destruction.
-    //
     return _instance->properties();
 }
 
 LoggerPtr
 Ice::CommunicatorI::getLogger() const
 {
-    //
-    // No check for destruction. It must be possible to access the
-    // logger after destruction.
-    //
     return _instance->logger();
 }
 
 void
 Ice::CommunicatorI::setLogger(const LoggerPtr& logger)
 {
-    //
-    // No check for destruction. It must be possible to set the logger
-    // after destruction (needed by logger plugins for example to
-    // unset the logger).
-    //
     _instance->logger(logger);
 }
 
 StatsPtr
 Ice::CommunicatorI::getStats() const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->stats();
 }
 
 void
 Ice::CommunicatorI::setStats(const StatsPtr& stats)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->stats(stats);
 }
 
 RouterPrx
 Ice::CommunicatorI::getDefaultRouter() const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->referenceFactory()->getDefaultRouter();
 }
 
 void
 Ice::CommunicatorI::setDefaultRouter(const RouterPrx& router)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->referenceFactory()->setDefaultRouter(router);
 }
 
 LocatorPrx
 Ice::CommunicatorI::getDefaultLocator() const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->referenceFactory()->getDefaultLocator();
 }
 
 void
 Ice::CommunicatorI::setDefaultLocator(const LocatorPrx& locator)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->referenceFactory()->setDefaultLocator(locator);
 }
 
 void
 Ice::CommunicatorI::setDefaultContext(const Context& ctx)
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     _instance->setDefaultContext(ctx);
 }
 
 Ice::Context
 Ice::CommunicatorI::getDefaultContext() const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->getDefaultContext();
 }
 
 PluginManagerPtr
 Ice::CommunicatorI::getPluginManager() const
 {
-    RecMutex::Lock sync(*this);
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
     return _instance->pluginManager();
 }
 
@@ -393,20 +253,19 @@ Ice::CommunicatorI::flushBatchRequests()
     _instance->flushBatchRequests();
 }
 
-Ice::CommunicatorI::CommunicatorI(const PropertiesPtr& properties) :
-    _destroyed(false)
+Ice::CommunicatorI::CommunicatorI(const PropertiesPtr& properties)
 {
     __setNoDelete(true);
     try
     {
-	_instance = new Instance(this, properties);
+	const_cast<InstancePtr&>(_instance) = new Instance(this, properties);
 
         //
         // Keep a reference to the dynamic library list to ensure
         // the libraries are not unloaded until this Communicator's
         // destructor is invoked.
         //
-        _dynamicLibraryList = _instance->dynamicLibraryList();
+	const_cast<DynamicLibraryListPtr&>(_dynamicLibraryList) = _instance->dynamicLibraryList();
     }
     catch(...)
     {
@@ -446,8 +305,7 @@ Ice::CommunicatorI::CommunicatorI(const PropertiesPtr& properties) :
 
 Ice::CommunicatorI::~CommunicatorI()
 {
-    RecMutex::Lock sync(*this);
-    if(!_destroyed)
+    if(!_instance->destroyed())
     {
 	Warning out(_instance->logger());
 	out << "Ice::Communicator::destroy() has not been called";
@@ -476,7 +334,6 @@ Ice::CommunicatorI::finishSetup(int& argc, char* argv[])
     }
     catch(...)
     {
-	_destroyed = true;
 	_instance->destroy();
 	throw;
     }
diff --git a/cpp/src/Ice/CommunicatorI.h b/cpp/src/Ice/CommunicatorI.h
index c74f5ccffa6..483e98ab4fd 100644
--- a/cpp/src/Ice/CommunicatorI.h
+++ b/cpp/src/Ice/CommunicatorI.h
@@ -74,15 +74,13 @@ private:
     friend ICE_API CommunicatorPtr initializeWithProperties(int&, char*[], const PropertiesPtr&, Int);
     friend ICE_API ::IceInternal::InstancePtr IceInternal::getInstance(const ::Ice::CommunicatorPtr&);
 
-    bool _destroyed;
-    ::IceInternal::InstancePtr _instance;
-    ::Ice::Context _dfltContext;
+    const ::IceInternal::InstancePtr _instance;
 
     //
     // We don't want the dynamic libraries to be unloaded until the
     // Communicator's destructor is invoked.
     //
-    ::IceInternal::DynamicLibraryListPtr _dynamicLibraryList;
+    const ::IceInternal::DynamicLibraryListPtr _dynamicLibraryList;
 };
 
 }
diff --git a/cpp/src/Ice/Instance.cpp b/cpp/src/Ice/Instance.cpp
index dd31dc1daf7..4a2c668c51f 100644
--- a/cpp/src/Ice/Instance.cpp
+++ b/cpp/src/Ice/Instance.cpp
@@ -68,6 +68,13 @@ extern bool ICE_UTIL_API nullHandleAbort;
 void IceInternal::incRef(Instance* p) { p->__incRef(); }
 void IceInternal::decRef(Instance* p) { p->__decRef(); }
 
+bool
+IceInternal::Instance::destroyed() const
+{
+    IceUtil::RecMutex::Lock sync(*this);
+    return _state == StateDestroyed;
+}
+
 CommunicatorPtr
 IceInternal::Instance::communicator() const
 {
@@ -77,32 +84,38 @@ IceInternal::Instance::communicator() const
 PropertiesPtr
 IceInternal::Instance::properties() const
 {
+    //
+    // No check for destruction. It must be possible to access the
+    // properties after destruction.
+    //
     // No mutex lock, immutable.
+    //
     return _properties;
 }
 
 LoggerPtr
 IceInternal::Instance::logger() const
 {
-    IceUtil::RecMutex::Lock sync(*this);
 
     //
-    // Don't throw CommunicatorDestroyedException if destroyed. We
-    // need the logger also after destructions.
+    // No check for destruction. It must be possible to access the
+    // logger after destruction.
     //
+
+    IceUtil::RecMutex::Lock sync(*this);
     return _logger;
 }
 
 void
 IceInternal::Instance::logger(const LoggerPtr& logger)
 {
-    IceUtil::RecMutex::Lock sync(*this);
-
-    if(_destroyed)
-    {
-	throw CommunicatorDestroyedException(__FILE__, __LINE__);
-    }
+    //
+    // No check for destruction. It must be possible to set the logger
+    // after destruction (needed by logger plugins for example to
+    // unset the logger).
+    //
 
+    IceUtil::RecMutex::Lock sync(*this);
     _logger = logger;
 }
 
@@ -111,7 +124,7 @@ IceInternal::Instance::stats() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -124,7 +137,7 @@ IceInternal::Instance::stats(const StatsPtr& stats)
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -151,7 +164,7 @@ IceInternal::Instance::routerManager() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -164,7 +177,7 @@ IceInternal::Instance::locatorManager() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -177,7 +190,7 @@ IceInternal::Instance::referenceFactory() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -190,7 +203,7 @@ IceInternal::Instance::proxyFactory() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -203,7 +216,7 @@ IceInternal::Instance::outgoingConnectionFactory() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -216,7 +229,7 @@ IceInternal::Instance::connectionMonitor() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -229,7 +242,7 @@ IceInternal::Instance::servantFactoryManager() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -242,7 +255,7 @@ IceInternal::Instance::objectAdapterFactory() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -255,7 +268,7 @@ IceInternal::Instance::clientThreadPool()
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -273,7 +286,7 @@ IceInternal::Instance::serverThreadPool()
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -306,7 +319,7 @@ IceInternal::Instance::endpointFactoryManager() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -319,7 +332,7 @@ IceInternal::Instance::dynamicLibraryList() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -332,7 +345,7 @@ IceInternal::Instance::pluginManager() const
 {
     IceUtil::RecMutex::Lock sync(*this);
 
-    if(_destroyed)
+    if(_state == StateDestroyed)
     {
 	throw CommunicatorDestroyedException(__FILE__, __LINE__);
     }
@@ -370,7 +383,7 @@ IceInternal::Instance::flushBatchRequests()
     {
 	IceUtil::RecMutex::Lock sync(*this);
 
-	if(_destroyed)
+	if(_state == StateDestroyed)
 	{
 	    throw CommunicatorDestroyedException(__FILE__, __LINE__);
 	}
@@ -386,19 +399,33 @@ IceInternal::Instance::flushBatchRequests()
 void
 IceInternal::Instance::setDefaultContext(const Context& ctx)
 {
+    IceUtil::RecMutex::Lock sync(*this);
+    
+    if(_state == StateDestroyed)
+    {
+	throw CommunicatorDestroyedException(__FILE__, __LINE__);
+    }
+
     _defaultContext = ctx;
 }
 
 const Context&
 IceInternal::Instance::getDefaultContext() const
 {
+    IceUtil::RecMutex::Lock sync(*this);
+    
+    if(_state == StateDestroyed)
+    {
+	throw CommunicatorDestroyedException(__FILE__, __LINE__);
+    }
+
     return _defaultContext;
 }
 
 
 IceInternal::Instance::Instance(const CommunicatorPtr& communicator, const PropertiesPtr& properties) :
     _communicator(communicator.get()),
-    _destroyed(false),
+    _state(StateActive),
     _properties(properties),
     _messageSizeMax(0),
     _clientACM(0),
@@ -406,7 +433,6 @@ IceInternal::Instance::Instance(const CommunicatorPtr& communicator, const Prope
     _threadPerConnection(false),
     _threadPerConnectionStackSize(0)
 {
-    
     try
     {
 	__setNoDelete(true);
@@ -641,7 +667,7 @@ IceInternal::Instance::Instance(const CommunicatorPtr& communicator, const Prope
 
 IceInternal::Instance::~Instance()
 {
-    assert(_destroyed);
+    assert(_state == StateDestroyed);
     assert(!_referenceFactory);
     assert(!_proxyFactory);
     assert(!_outgoingConnectionFactory);
@@ -761,10 +787,29 @@ IceInternal::Instance::finishSetup(int& argc, char* argv[])
     //
 }
 
-void
+bool
 IceInternal::Instance::destroy()
 {
-    assert(!_destroyed);
+    {
+	IceUtil::RecMutex::Lock sync(*this);
+	
+	//
+	// If the _state is not StateActive then the instance is
+	// either being destroyed, or has already been destroyed.
+	//
+	if(_state != StateActive)
+	{
+	    return false;
+	}
+
+	//
+	// We cannot set state to StateDestroyed otherwise instance
+	// methods called during the destroy process (such as
+	// outgoingConnectionFactory() from
+	// ObjectAdapterI::deactivate() will cause an exception.
+	//
+	_state = StateDestroyInProgress;
+    }
 
     if(_objectAdapterFactory)
     {
@@ -857,7 +902,7 @@ IceInternal::Instance::destroy()
 	// _dynamicLibraryList->destroy();
 	_dynamicLibraryList = 0;
 
-	_destroyed = true;
+	_state = StateDestroyed;
     }
 
     //
@@ -870,5 +915,6 @@ IceInternal::Instance::destroy()
     if(serverThreadPool)
     {
 	serverThreadPool->joinWithAllThreads();
-    }
+    }

+    return true;
 }
diff --git a/cpp/src/Ice/Instance.h b/cpp/src/Ice/Instance.h
index ca0b971282a..d54d838e1d3 100644
--- a/cpp/src/Ice/Instance.h
+++ b/cpp/src/Ice/Instance.h
@@ -48,6 +48,7 @@ class Instance : public IceUtil::Shared, public IceUtil::RecMutex
 {
 public:
 
+    bool destroyed() const;
     Ice::CommunicatorPtr communicator() const;
     Ice::PropertiesPtr properties() const;
     Ice::LoggerPtr logger() const;
@@ -83,11 +84,17 @@ private:
     Instance(const Ice::CommunicatorPtr&, const Ice::PropertiesPtr&);
     virtual ~Instance();
     void finishSetup(int&, char*[]);
-    void destroy();
+    bool destroy();
     friend class Ice::CommunicatorI;
 
     Ice::Communicator* _communicator; // Not a Ptr, to avoid having Instance and CommunicatorI point at each other.
-    bool _destroyed;
+    enum State
+    {
+	StateActive,
+	StateDestroyInProgress,
+	StateDestroyed
+    };
+    State _state;
     const Ice::PropertiesPtr _properties; // Immutable, not reset by destroy().
     Ice::LoggerPtr _logger; // Not reset by destroy().
     Ice::StatsPtr _stats; // Not reset by destroy().