Skip to content

Commit

Permalink
Backport fix for Swift race condition (#3054)
Browse files Browse the repository at this point in the history
  • Loading branch information
externl authored Nov 6, 2024
1 parent d27d388 commit a599733
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 14 deletions.
3 changes: 3 additions & 0 deletions swift/src/IceImpl/BlobjectFacade.mm
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
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
Expand Down
53 changes: 39 additions & 14 deletions swift/src/IceImpl/LocalObject.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -26,8 +27,8 @@ -(instancetype) initWithCppObject:(std::shared_ptr<void>)cppObject

@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;
}
Expand All @@ -40,24 +41,48 @@ +(nullable instancetype) getHandle:(std::shared_ptr<void>)cppObject
}
@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);
}
}
}
}

Expand Down

0 comments on commit a599733

Please sign in to comment.