Fixed problem with GC singleton that happened if a communicator was

    destroyed recreated, so the number of communicators transitioned 0 -> 1
    -> 0 -> 1.
Also fixed a race condition that caused a deadlock: if communicators were
    created and destroyed in quick succession, a notify() on the GC thread
    was lost, causing GC::stop() to hang indefinitely.

2 files changed, 62 insertions, 40 deletions

diff --git a/cpp/src/Ice/CommunicatorI.cpp b/cpp/src/Ice/CommunicatorI.cpp
index a3d869e1990..f4fc8d20d3c 100644
--- a/cpp/src/Ice/CommunicatorI.cpp
+++ b/cpp/src/Ice/CommunicatorI.cpp
@@ -54,12 +54,11 @@ static GarbageCollectorStats gcStats;
 static int gcTraceLevel;
 static string gcTraceCat;
 static LoggerPtr gcLogger;
+static int gcInterval;
 
 static void
 printGCStats(const ::IceUtil::GCStats& stats)
 {
-    IceUtil::StaticMutex::Lock l(gcMutex);
-
     if(gcTraceLevel)
     {
 	if(gcTraceLevel > 1)
@@ -89,39 +88,33 @@ Ice::CommunicatorI::destroy()
 	}
     }
 
-    bool last;
     {
 	IceUtil::StaticMutex::Lock sync(gcMutex);
-        last = (--communicatorCount == 0);
-    }
 
-    if(last)
-    {
 	//
 	// Wait for the collector thread to stop if this is the last communicator
 	// to be destroyed.
 	//
-	theCollector->stop();
-    }
-
-    theCollector->collectGarbage(); // Collect whenever a communicator is destroyed.
-
-    if(last)
-    {
-	IceUtil::StaticMutex::Lock l(gcMutex);
-
-	if(gcTraceLevel)
+	bool last = (--communicatorCount == 0);
+	if(last && gcInterval > 0)
 	{
-	    Trace out(gcLogger, gcTraceCat);
-	    out << "totals: " << gcStats.collected << "/" << gcStats.examined << ", "
-		<< gcStats.msec << "ms" << ", " << gcStats.runs << " run";
-	    if(gcStats.runs != 1)
+	    theCollector->stop();
+	}
+	theCollector->collectGarbage(); // Collect whenever a communicator is destroyed.
+	if(last)
+	{
+	    if(gcTraceLevel)
 	    {
-		out << "s";
+		Trace out(gcLogger, gcTraceCat);
+		out << "totals: " << gcStats.collected << "/" << gcStats.examined << ", "
+		    << gcStats.msec << "ms" << ", " << gcStats.runs << " run";
+		if(gcStats.runs != 1)
+		{
+		    out << "s";
+		}
 	    }
+	    theCollector = 0; // Force destruction of the collector.
 	}
-	gcTraceLevel = 0;
-	gcLogger = 0;
     }
 
     if(instance)
@@ -376,13 +369,22 @@ Ice::CommunicatorI::CommunicatorI(int& argc, char* argv[], const PropertiesPtr&
 	// is created isn't the last communicator to be destroyed.
 	//
 	IceUtil::StaticMutex::Lock sync(gcMutex);
-	if(++communicatorCount == 1)
+	static bool gcOnce = true;
+	if(gcOnce)
 	{
 	    gcTraceLevel = _instance->traceLevels()->gc;
 	    gcTraceCat = _instance->traceLevels()->gcCat;
 	    gcLogger = _instance->logger();
-	    theCollector = new IceUtil::GC(properties->getPropertyAsInt("Ice.GC.Interval"), printGCStats);
-	    theCollector->start();
+	    gcInterval = properties->getPropertyAsInt("Ice.GC.Interval");
+	    gcOnce = false;
+	}
+	if(++communicatorCount == 1)
+	{
+	    theCollector = new IceUtil::GC(gcInterval, printGCStats);
+	    if(gcInterval > 0)
+	    {
+		theCollector->start();
+	    }
 	}
     }
 }
diff --git a/cpp/src/IceUtil/GC.cpp b/cpp/src/IceUtil/GC.cpp
index 09f757f6dae..cd8b23a4298 100644
--- a/cpp/src/IceUtil/GC.cpp
+++ b/cpp/src/IceUtil/GC.cpp
@@ -46,13 +46,13 @@ int IceUtil::GC::_numCollectors = 0;
 
 IceUtil::GC::GC(int interval, StatsCallback cb)
 {
+    Monitor<Mutex>::Lock sync(*this);
+
     if(_numCollectors++ > 0)
     {
 	abort(); // Enforce singleton.
     }
-
-    Monitor<Mutex>::Lock sync(*this);
-    _running = false;
+    _state = NotStarted;
     _collecting = false;
     _interval = interval;
     _statsCallback = cb;
@@ -68,23 +68,25 @@ IceUtil::GC::~GC()
 void
 IceUtil::GC::run()
 {
+    assert(_interval > 0);
+
     {
 	Monitor<Mutex>::Lock sync(*this);
-	if(_interval == 0)
-	{
-	    return;
-	}
-	_running = true;
+
+	_state = Started;
+	notify();
     }
 
+    Time waitTime = Time::seconds(_interval);
     while(true)
     {
 	bool collect = false;
 	{
-	    Time waitTime = Time::seconds(_interval);
 	    Monitor<Mutex>::Lock sync(*this);
-	    if(!_running)
+
+	    if(_state == Stopping)
 	    {
+		_state = Stopped;
 		return;
 	    }
 	    if(!timedWait(waitTime))
@@ -104,17 +106,35 @@ IceUtil::GC::stop()
 {
     {
 	Monitor<Mutex>::Lock sync(*this);
-	if(!_running)
+
+	if(_state >= Stopping)
 	{
-	    return;
+	    return; // Don't attempt to stop the thread twice.
+	}
+
+	//
+	// Wait until the thread is actually started. (If we don't do this, we
+	// can get a problem if a call to stop() immediately follows a call to start():
+	// the call to stop() may happen before pthread_create() has scheduled the thread's run()
+	// function, and then the notify() that is used to tell the thread to stop can be lost.
+	//
+	while(_state < Started)
+	{
+	    wait();
 	}
     }
+
+    //
+    // Tell the thread to stop.
+    //
     {
 	Monitor<Mutex>::Lock sync(*this);
-	_running = false;
+	_state = Stopping;
 	notify();
     }
+
     getThreadControl().join();
+    assert(_state == Stopped);
 }
 
 void