Added a comment to GCShared to explain why delete is called by __decRef()

    outside the protection of the lock.
Also moved the code to update the object set from ~GCShared() to the
    garbage collector. That's cleaner than doing it from the destructor.

2 files changed, 7 insertions, 3 deletions

diff --git a/cpp/src/IceUtil/GC.cpp b/cpp/src/IceUtil/GC.cpp
index cd8b23a4298..148a85fb878 100644
--- a/cpp/src/IceUtil/GC.cpp
+++ b/cpp/src/IceUtil/GC.cpp
@@ -229,6 +229,7 @@ IceUtil::GC::collectGarbage()
 	}
 	for(i = counts.begin(); i != counts.end(); ++i)
 	{
+	    gcObjects.erase(i->first);
 	    delete i->first;
 	    if(_statsCallback)
 	    {
diff --git a/cpp/src/IceUtil/GCShared.cpp b/cpp/src/IceUtil/GCShared.cpp
index 5a92cdaeb76..b3828475481 100644
--- a/cpp/src/IceUtil/GCShared.cpp
+++ b/cpp/src/IceUtil/GCShared.cpp
@@ -29,9 +29,6 @@ IceUtil::GCShared::GCShared()
 
 IceUtil::GCShared::~GCShared()
 {
-    gcRecMutex._m->lock();
-    gcObjects.erase(this);
-    gcRecMutex._m->unlock();
 }
 
 void
@@ -71,6 +68,12 @@ IceUtil::GCShared::__decRef()
     }
     gcRecMutex._m->unlock();
 
+    //
+    // We call delete outside the lock to avoid deadlock: if thread 1 executes
+    // the destructor of a class, and the destructor tries to join with thread 2,
+    // we would deadlock if thread 2 can't complete until after it has destroyed
+    // some class instance of its own.
+    //
     if(doDelete)
     {
 	delete this;