[Darwin] more XPC service tweaks (#35048)

* return `MTRDevice_XPC`s from XPC controller

* more logging

* move shadow property declarations to internal header

* declare `_setupDeviceForNodeID` as common internal device controller method

* prefetchedClusterData is nullable

* fix a few properties that needed raising to base class

* you get a log and you get a log EVERYONE GETS A LOG

* convert device map lock for use in subclasses

* check for optional delegate method impl before calling

* ivar no longer necessary with accessor method

underlying lock is the only state needed

* Restyled by clang-format

* remove more obsolete lock bits from `MTRDeviceController_XPC`

---------

Co-authored-by: Restyled.io <commits@restyled.io>

Author: kiel-apple

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -120,7 +120,7 @@ @implementation MTRDeviceController {
     MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
     MTRDeviceControllerFactory * _factory;
     NSMapTable * _nodeIDToDeviceMap;
-    os_unfair_lock _deviceMapLock; // protects nodeIDToDeviceMap
+    os_unfair_lock _underlyingDeviceMapLock;
     MTRCommissionableBrowser * _commissionableBrowser;
     MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;
 
@@ -134,12 +134,17 @@ @implementation MTRDeviceController {
     MTRP256KeypairBridge _operationalKeypairBridge;
 }
 
+- (os_unfair_lock_t)deviceMapLock
+{
+    return &_underlyingDeviceMapLock;
+}
+
 - (instancetype)initForSubclasses
 {
     if (self = [super init]) {
         // nothing, as superclass of MTRDeviceController is NSObject
     }
-
+    _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT;
     return self;
 }
 
@@ -250,7 +255,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
 
         _chipWorkQueue = queue;
         _factory = factory;
-        _deviceMapLock = OS_UNFAIR_LOCK_INIT;
+        _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT;
         _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
         _serverEndpoints = [[NSMutableArray alloc] init];
         _commissionableBrowser = nil;
@@ -330,10 +335,10 @@ - (void)cleanupAfterStartup
     // while calling out into arbitrary invalidation code, snapshot the list of
     // devices before we start invalidating.
     MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self);
-    os_unfair_lock_lock(&_deviceMapLock);
+    os_unfair_lock_lock(self.deviceMapLock);
     NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator];
     [_nodeIDToDeviceMap removeAllObjects];
-    os_unfair_lock_unlock(&_deviceMapLock);
+    os_unfair_lock_unlock(self.deviceMapLock);
 
     for (MTRDevice * device in devices) {
         [device invalidate];
@@ -634,7 +639,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
         [_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary<NSNumber *, NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *> * _Nonnull clusterDataByNode) {
             MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast<unsigned long>(clusterDataByNode.count), self->_uniqueIdentifier);
 
-            std::lock_guard lock(self->_deviceMapLock);
+            std::lock_guard lock(*self.deviceMapLock);
             NSMutableArray * deviceList = [NSMutableArray array];
             for (NSNumber * nodeID in clusterDataByNode) {
                 NSDictionary * clusterData = clusterDataByNode[nodeID];
@@ -1004,7 +1009,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
 // If prefetchedClusterData is not provided, load attributes individually from controller data store
 - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
 {
-    os_unfair_lock_assert_owner(&_deviceMapLock);
+    os_unfair_lock_assert_owner(self.deviceMapLock);
 
     MTRDevice * deviceToReturn = [[MTRDevice_Concrete alloc] initWithNodeID:nodeID controller:self];
     // If we're not running, don't add the device to our map.  That would
@@ -1043,7 +1048,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N
 
 - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
 {
-    std::lock_guard lock(_deviceMapLock);
+    std::lock_guard lock(*self.deviceMapLock);
     MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID];
     if (!deviceToReturn) {
         deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil];
@@ -1054,7 +1059,7 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
 
 - (void)removeDevice:(MTRDevice *)device
 {
-    std::lock_guard lock(_deviceMapLock);
+    std::lock_guard lock(*self.deviceMapLock);
     auto * nodeID = device.nodeID;
     MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID];
     if (deviceToRemove == device) {
@@ -1068,7 +1073,7 @@ - (void)removeDevice:(MTRDevice *)device
 #ifdef DEBUG
 - (NSDictionary<NSNumber *, NSNumber *> *)unitTestGetDeviceAttributeCounts
 {
-    std::lock_guard lock(_deviceMapLock);
+    std::lock_guard lock(*self.deviceMapLock);
     NSMutableDictionary<NSNumber *, NSNumber *> * deviceAttributeCounts = [NSMutableDictionary dictionary];
     for (NSNumber * nodeID in _nodeIDToDeviceMap) {
         deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
@@ -1508,9 +1513,9 @@ - (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);
+    os_unfair_lock_lock(self.deviceMapLock);
     MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)];
-    os_unfair_lock_unlock(&_deviceMapLock);
+    os_unfair_lock_unlock(self.deviceMapLock);
 
     if (device == nil) {
         return;

src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm

@@ -82,8 +82,6 @@
 #include <dns_sd.h>
 #include <string>
 
-#import <os/lock.h>
-
 typedef void (^SyncWorkQueueBlock)(void);
 typedef id (^SyncWorkQueueBlockWithReturnValue)(void);
 typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void);
@@ -103,8 +101,6 @@ @interface MTRDeviceController_Concrete ()
 @property (nonatomic, readwrite) NSUUID * uniqueIdentifier;
 @property (nonatomic, readonly) dispatch_queue_t chipWorkQueue;
 @property (nonatomic, readonly, nullable) MTRDeviceControllerFactory * factory;
-@property (nonatomic, readonly, nullable) NSMapTable * nodeIDToDeviceMap;
-@property (nonatomic, readonly) os_unfair_lock deviceMapLock;
 @property (nonatomic, readonly, nullable) id<MTROTAProviderDelegate> otaProviderDelegate;
 @property (nonatomic, readonly, nullable) dispatch_queue_t otaProviderDelegateQueue;
 @property (nonatomic, readonly, nullable) MTRCommissionableBrowser * commissionableBrowser;
@@ -270,11 +266,9 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
 
         _otaProviderDelegate = otaProviderDelegate;
         _otaProviderDelegateQueue = otaProviderDelegateQueue;
-
         _chipWorkQueue = queue;
         _factory = factory;
-        _deviceMapLock = OS_UNFAIR_LOCK_INIT;
-        _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
+        self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
         _serverEndpoints = [[NSMutableArray alloc] init];
         _commissionableBrowser = nil;
 
@@ -352,10 +346,10 @@ - (void)cleanupAfterStartup
     // while calling out into arbitrary invalidation code, snapshot the list of
     // devices before we start invalidating.
     MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self);
-    os_unfair_lock_lock(&_deviceMapLock);
-    NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator];
-    [_nodeIDToDeviceMap removeAllObjects];
-    os_unfair_lock_unlock(&_deviceMapLock);
+    os_unfair_lock_lock(self.deviceMapLock);
+    NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
+    [self.nodeIDToDeviceMap removeAllObjects];
+    os_unfair_lock_unlock(self.deviceMapLock);
 
     for (MTRDevice * device in devices) {
         [device invalidate];
@@ -654,7 +648,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
         [_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary<NSNumber *, NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *> * _Nonnull clusterDataByNode) {
             MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast<unsigned long>(clusterDataByNode.count), self->_uniqueIdentifier);
 
-            std::lock_guard lock(self->_deviceMapLock);
+            std::lock_guard lock(*self.deviceMapLock);
             NSMutableArray * deviceList = [NSMutableArray array];
             for (NSNumber * nodeID in clusterDataByNode) {
                 NSDictionary * clusterData = clusterDataByNode[nodeID];
@@ -1024,15 +1018,15 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
 // If prefetchedClusterData is not provided, load attributes individually from controller data store
 - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
 {
-    os_unfair_lock_assert_owner(&_deviceMapLock);
+    os_unfair_lock_assert_owner(self.deviceMapLock);
 
     MTRDevice * deviceToReturn = [[MTRDevice alloc] initWithNodeID:nodeID controller:self];
     // If we're not running, don't add the device to our map.  That would
     // create a cycle that nothing would break.  Just return the device,
     // 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 setObject:deviceToReturn forKey:nodeID];
+        [self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
     }
 
     if (prefetchedClusterData) {
@@ -1063,8 +1057,8 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N
 
 - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
 {
-    std::lock_guard lock(_deviceMapLock);
-    MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID];
+    std::lock_guard lock(*self.deviceMapLock);
+    MTRDevice * deviceToReturn = [self.nodeIDToDeviceMap objectForKey:nodeID];
     if (!deviceToReturn) {
         deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil];
     }
@@ -1074,12 +1068,12 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
 
 - (void)removeDevice:(MTRDevice *)device
 {
-    std::lock_guard lock(_deviceMapLock);
+    std::lock_guard lock(*self.deviceMapLock);
     auto * nodeID = device.nodeID;
-    MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID];
+    MTRDevice * deviceToRemove = [self.nodeIDToDeviceMap objectForKey:nodeID];
     if (deviceToRemove == device) {
         [deviceToRemove invalidate];
-        [_nodeIDToDeviceMap removeObjectForKey:nodeID];
+        [self.nodeIDToDeviceMap removeObjectForKey:nodeID];
     } else {
         MTR_LOG_ERROR("%@ Error: Cannot remove device %p with nodeID %llu", self, device, nodeID.unsignedLongLongValue);
     }
@@ -1088,10 +1082,10 @@ - (void)removeDevice:(MTRDevice *)device
 #ifdef DEBUG
 - (NSDictionary<NSNumber *, NSNumber *> *)unitTestGetDeviceAttributeCounts
 {
-    std::lock_guard lock(_deviceMapLock);
+    std::lock_guard lock(*self.deviceMapLock);
     NSMutableDictionary<NSNumber *, NSNumber *> * deviceAttributeCounts = [NSMutableDictionary dictionary];
-    for (NSNumber * nodeID in _nodeIDToDeviceMap) {
-        deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
+    for (NSNumber * nodeID in self.nodeIDToDeviceMap) {
+        deviceAttributeCounts[nodeID] = @([[self.nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
     }
     return deviceAttributeCounts;
 }
@@ -1540,9 +1534,9 @@ - (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 objectForKey:@(nodeID)];
-    os_unfair_lock_unlock(&_deviceMapLock);
+    os_unfair_lock_lock(self.deviceMapLock);
+    MTRDevice * device = [self.nodeIDToDeviceMap objectForKey:@(nodeID)];
+    os_unfair_lock_unlock(self.deviceMapLock);
 
     if (device == nil) {
         return;

src/darwin/Framework/CHIP/MTRDeviceController_Internal.h

@@ -29,6 +29,8 @@
 #include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
 
+#import <os/lock.h>
+
 #import "MTRBaseDevice.h"
 #import "MTRDeviceController.h"
 #import "MTRDeviceControllerDataStore.h"
@@ -63,6 +65,9 @@ NS_ASSUME_NONNULL_BEGIN
 
 @interface MTRDeviceController ()
 
+@property (nonatomic, readwrite, nullable) NSMapTable * nodeIDToDeviceMap;
+@property (readonly, assign) os_unfair_lock_t deviceMapLock;
+
 - (instancetype)initForSubclasses;
 
 #pragma mark - MTRDeviceControllerFactory methods
@@ -270,6 +275,7 @@ NS_ASSUME_NONNULL_BEGIN
 #pragma mark - Device-specific data and SDK access
 // DeviceController will act as a central repository for this opaque dictionary that MTRDevice manages
 - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID;
+- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData;
 - (void)removeDevice:(MTRDevice *)device;
 
 /**

src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm

@@ -15,6 +15,7 @@
  */
 
 #import "MTRDeviceController_XPC.h"
+
 #import "MTRDefines_Internal.h"
 #import "MTRDeviceController_Internal.h"
 #import "MTRDevice_XPC.h"
@@ -45,7 +46,7 @@ @implementation MTRDeviceController_XPC
 - (id)initWithUniqueIdentifier:(NSUUID *)UUID xpConnectionBlock:(NSXPCConnection * (^)(void) )connectionBlock
 {
     if (self = [super initForSubclasses]) {
-        MTR_LOG("Setting up XPC Controller for UUID: %@  with connection block: %p", UUID, connectionBlock);
+        MTR_LOG("Setting up XPC Controller for UUID: %@ with connection block: %p", UUID, connectionBlock);
 
         if (UUID == nil) {
             MTR_LOG_ERROR("MTRDeviceController_XPC initWithUniqueIdentifier failed, nil UUID");
@@ -111,6 +112,24 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete
     return nil;
 }
 
+// If prefetchedClusterData is not provided, load attributes individually from controller data store
+- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
+{
+    MTR_LOG("%s", __PRETTY_FUNCTION__);
+    os_unfair_lock_assert_owner(self.deviceMapLock);
+
+    MTRDevice * deviceToReturn = [[MTRDevice_XPC alloc] initWithNodeID:nodeID controller:self];
+    // If we're not running, don't add the device to our map.  That would
+    // create a cycle that nothing would break.  Just return the device,
+    // 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]) {
+        [self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
+    }
+    MTR_LOG("%s: returning XPC device for node id %@", __PRETTY_FUNCTION__, nodeID);
+    return deviceToReturn;
+}
+
 #pragma mark - XPC Action Overrides
 
 MTR_DEVICECONTROLLER_SIMPLE_REMOTE_XPC_GETTER(isRunning, BOOL, NO, getIsRunningWithReply)

src/darwin/Framework/CHIP/MTRDevice_XPC.mm

@@ -82,41 +82,59 @@
 
 @implementation MTRDevice_XPC
 
-#pragma mark - Client Callbacks
+#pragma mark - Client Callbacks (MTRDeviceDelegate)
+
+// required methods for MTRDeviceDelegates
 - (oneway void)device:(NSNumber *)nodeID stateChanged:(MTRDeviceState)state
 {
+    MTR_LOG("%s", __PRETTY_FUNCTION__);
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
         [delegate device:self stateChanged:state];
     }];
 }
+
 - (oneway void)device:(NSNumber *)nodeID receivedAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport
 {
+    MTR_LOG("%s", __PRETTY_FUNCTION__);
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
         [delegate device:self receivedAttributeReport:attributeReport];
     }];
 }
+
 - (oneway void)device:(NSNumber *)nodeID receivedEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport
 {
+    MTR_LOG("%s", __PRETTY_FUNCTION__);
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
         [delegate device:self receivedEventReport:eventReport];
     }];
 }
+
+// optional methods for MTRDeviceDelegates - check for implementation before calling
 - (oneway void)deviceBecameActive:(NSNumber *)nodeID
 {
+    MTR_LOG("%s", __PRETTY_FUNCTION__);
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
-        [delegate deviceBecameActive:self];
+        if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) {
+            [delegate deviceBecameActive:self];
+        }
     }];
 }
+
 - (oneway void)deviceCachePrimed:(NSNumber *)nodeID
 {
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
-        [delegate deviceCachePrimed:self];
+        if ([delegate respondsToSelector:@selector(deviceCachePrimed:)]) {
+            [delegate deviceCachePrimed:self];
+        }
     }];
 }
+
 - (oneway void)deviceConfigurationChanged:(NSNumber *)nodeID
 {
     [self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
-        [delegate deviceConfigurationChanged:self];
+        if ([delegate respondsToSelector:@selector(deviceConfigurationChanged:)]) {
+            [delegate deviceConfigurationChanged:self];
+        }
     }];
 }