Backport fix for Swift race condition (#3054)

2 files changed, 42 insertions, 14 deletions

diff --git a/swift/src/IceImpl/BlobjectFacade.mm b/swift/src/IceImpl/BlobjectFacade.mm
index 446535fdd30..f4f6189f9ce 100644
--- a/swift/src/IceImpl/BlobjectFacade.mm
+++ b/swift/src/IceImpl/BlobjectFacade.mm
@@ -26,6 +26,9 @@ BlobjectFacade::ice_invokeAsync(std::pair<const Byte*, const Byte*> inEncaps,
     ICEObjectAdapter* adapter = [ICEObjectAdapter getHandle:current.adapter];
     ICEConnection* con = [ICEConnection getHandle:current.con];
 
+    // Both are null (colloc) or both are non-null (non-colloc).
+    assert((current.con && con) || (!current.con && !con));
+
     @autoreleasepool
     {
         [_facade facadeInvoke:adapter
diff --git a/swift/src/IceImpl/LocalObject.mm b/swift/src/IceImpl/LocalObject.mm
index 092397a308a..7227ea71773 100644
--- a/swift/src/IceImpl/LocalObject.mm
+++ b/swift/src/IceImpl/LocalObject.mm
@@ -8,7 +8,8 @@
 
 namespace
 {
-    std::unordered_map<void*, __weak ICELocalObject*> cachedObjects;
+    // We "leak" this map to avoid the destructor being called when the application is terminated.
+    auto* cachedObjects = new std::unordered_map<void*, __weak ICELocalObject*>();
 }
 
 @implementation ICELocalObject
@@ -26,8 +27,8 @@ namespace
 
     @synchronized([ICELocalObject class])
     {
-        assert(cachedObjects.find(_cppObject.get()) == cachedObjects.end());
-        cachedObjects.insert(std::make_pair(_cppObject.get(), self));
+        assert(cachedObjects->find(_cppObject.get()) == cachedObjects->end());
+        cachedObjects->insert(std::make_pair(_cppObject.get(), self));
     }
     return self;
 }
@@ -40,24 +41,48 @@ namespace
     }
     @synchronized([ICELocalObject class])
     {
-        std::unordered_map<void*, __weak ICELocalObject*>::const_iterator p = cachedObjects.find(cppObject.get());
-        if(p != cachedObjects.end())
+        auto p = cachedObjects->find(cppObject.get());
+        if (p != cachedObjects->end())
         {
-            return p->second;
-        }
-        else
-        {
-            return [[[self class] alloc] initWithCppObject:std::move(cppObject)];
+            // Get a strong reference to the object. If it's nil, preemptively remove it from the cache,
+            // otherwise we'll get an assert when we try to init a new one.
+            // This can happen if the object is being deallocated on another thread.
+            ICELocalObject* obj = p->second;
+            if (obj == nil)
+            {
+                cachedObjects->erase(p);
+            }
+            else
+            {
+                return obj;
+            }
         }
+
+        return [[[self class] alloc] initWithCppObject:std::move(cppObject)];
     }
 }
 
--(void) dealloc {
-    assert(_cppObject != nullptr);
+- (void)dealloc
+{
     @synchronized([ICELocalObject class])
     {
-        cachedObjects.erase(_cppObject.get());
-        _cppObject = nullptr;
+        assert(_cppObject != nullptr);
+
+        // Find the object in the cache. If it's not there, it's likely that another thread already replaced
+        // the object with a new one and then that new one was quickly deallocated.
+        if (auto p = cachedObjects->find(_cppObject.get()); p != cachedObjects->end())
+        {
+            // The object in the cache is either nil or NOT the current object. The later can happen if this thread was
+            // trying to deallocate the object while another thread was trying to create a new one.
+            assert(p->second == nil || p->second != self);
+
+            // When the last reference on this object is released, p->second is nil and we remove the stale entry from
+            // the cache.
+            if (p->second == nil)
+            {
+                cachedObjects->erase(p);
+            }
+        }
     }
 }