Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving to NSMapTable to avoid dangling MTRDevices #33932

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Moving to NSMapTable to avoid dangling MTRDevices #33932

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fe346f
Moving to NSMapTable
woody-apple Jun 14, 2024
355f452
Restyled by clang-format
restyled-commits Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ @implementation MTRDeviceController {
MTRP256KeypairBridge _operationalKeypairBridge;
MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
MTRDeviceControllerFactory * _factory;
NSMutableDictionary * _nodeIDToDeviceMap;
NSMapTable * _nodeIDToDeviceMap;
woody-apple marked this conversation as resolved.
Show resolved Hide resolved
os_unfair_lock _deviceMapLock; // protects nodeIDToDeviceMap
MTRCommissionableBrowser * _commissionableBrowser;
MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;
Expand Down Expand Up @@ -236,7 +236,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
_chipWorkQueue = queue;
_factory = factory;
_deviceMapLock = OS_UNFAIR_LOCK_INIT;
_nodeIDToDeviceMap = [NSMutableDictionary dictionary];
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
_serverEndpoints = [[NSMutableArray alloc] init];
_commissionableBrowser = nil;

Expand Down Expand Up @@ -307,7 +307,7 @@ - (void)cleanupAfterStartup
// while calling out into arbitrary invalidation code, snapshot the list of
// devices before we start invalidating.
os_unfair_lock_lock(&_deviceMapLock);
NSArray<MTRDevice *> * devices = [_nodeIDToDeviceMap allValues];
NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator];
[_nodeIDToDeviceMap removeAllObjects];
os_unfair_lock_unlock(&_deviceMapLock);

Expand Down Expand Up @@ -970,7 +970,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N
// which will be in exactly the state it would be in if it were created
// while we were running and then we got shut down.
if ([self isRunning]) {
_nodeIDToDeviceMap[nodeID] = deviceToReturn;
[_nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
}

if (prefetchedClusterData) {
Expand Down Expand Up @@ -1002,7 +1002,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N
- (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
{
std::lock_guard lock(_deviceMapLock);
MTRDevice * deviceToReturn = _nodeIDToDeviceMap[nodeID];
MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID];
if (!deviceToReturn) {
deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil];
}
Expand All @@ -1014,10 +1014,10 @@ - (void)removeDevice:(MTRDevice *)device
{
std::lock_guard lock(_deviceMapLock);
auto * nodeID = device.nodeID;
MTRDevice * deviceToRemove = _nodeIDToDeviceMap[nodeID];
MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID];
if (deviceToRemove == device) {
[deviceToRemove invalidate];
_nodeIDToDeviceMap[nodeID] = nil;
[_nodeIDToDeviceMap removeObjectForKey:nodeID];
} else {
MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, nodeID.unsignedLongLongValue);
}
Expand All @@ -1029,7 +1029,7 @@ - (void)removeDevice:(MTRDevice *)device
std::lock_guard lock(_deviceMapLock);
NSMutableDictionary<NSNumber *, NSNumber *> * deviceAttributeCounts = [NSMutableDictionary dictionary];
for (NSNumber * nodeID in _nodeIDToDeviceMap) {
deviceAttributeCounts[nodeID] = @([_nodeIDToDeviceMap[nodeID] unitTestAttributeCount]);
deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
}
return deviceAttributeCounts;
}
Expand Down Expand Up @@ -1260,7 +1260,7 @@ - (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConn
// First check if MTRDevice exists from having loaded from storage, or created by a client.
// Do not use deviceForNodeID here, because we don't want to create the device if it does not already exist.
os_unfair_lock_lock(&_deviceMapLock);
MTRDevice * device = _nodeIDToDeviceMap[@(nodeID)];
MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)];
os_unfair_lock_unlock(&_deviceMapLock);

// In the case that this device is known to use thread, queue this with subscription attempts as well, to
Expand Down Expand Up @@ -1482,7 +1482,7 @@ - (void)operationalInstanceAdded:(chip::NodeId)nodeID
// Don't use deviceForNodeID here, because we don't want to create the
// device if it does not already exist.
os_unfair_lock_lock(&_deviceMapLock);
MTRDevice * device = _nodeIDToDeviceMap[@(nodeID)];
MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)];
os_unfair_lock_unlock(&_deviceMapLock);

if (device == nil) {
Expand Down
Loading