Make queue handling consistent for commands in MTRDevice/MTRClusters. (#23959)

invokeCommandWithEndpointID on MTRDevice ran the async work item management
logic on the MTRDevice's work queue, and queued an async callback to the client
queue for the command response.  This was different from what MTRClusters did:
that ran the management logic on the client queue as well.

This PR aligns on the invokeCommandWithEndpointID behavior, because the
MTRClusters behavior allows a poorly-behaved client that blocks inside the
completion handler to completely block the device's async callback queue
processing.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -130,7 +130,6 @@ - (id)strongObject
 #pragma mark - MTRDevice
 @interface MTRDevice ()
 @property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state
-@property (nonatomic) dispatch_queue_t queue;
 @property (nonatomic) MTRWeakReference<id<MTRDeviceDelegate>> * weakDelegate;
 @property (nonatomic) dispatch_queue_t delegateQueue;
 @property (nonatomic) NSArray<NSDictionary<NSString *, id> *> * unreportedEvents;
@@ -156,8 +155,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle
         _lock = OS_UNFAIR_LOCK_INIT;
         _nodeID = [nodeID copy];
         _deviceController = controller;
-        _queue = dispatch_queue_create("com.apple.matter.framework.xpc.workqueue", DISPATCH_QUEUE_SERIAL);
-        ;
+        _queue = dispatch_queue_create("com.apple.matter.framework.device.workqueue", DISPATCH_QUEUE_SERIAL);
         _readCache = [NSMutableDictionary dictionary];
         _expectedValueCache = [NSMutableDictionary dictionary];
         _asyncCallbackWorkQueue = [[MTRAsyncCallbackWorkQueue alloc] initWithContext:self queue:_queue];
@@ -413,7 +411,7 @@ - (void)setupSubscription
     NSString * logPrefix = [NSString
         stringWithFormat:@"MTRDevice read %u %@ %@ %@", _deviceController.fabricIndex, endpointID, clusterID, attributeID];
     // Create work item, set ready handler to perform task, then enqueue the work
-    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
+    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
     MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
         MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
         MTRBaseDevice * baseDevice = [self newBaseDevice];
@@ -468,7 +466,7 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
         timeout = MTRClampedNumber(timeout, @(1), @(UINT16_MAX));
     }
     expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
-    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
+    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
     MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
         MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
         MTRBaseDevice * baseDevice = [self newBaseDevice];
@@ -517,7 +515,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
     } else {
         expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
     }
-    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
+    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
     MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
         MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
         MTRBaseDevice * baseDevice = [self newBaseDevice];
@@ -611,7 +609,7 @@ - (void)_checkExpiredExpectedValues
             waitTime = MTR_DEVICE_EXPIRATION_CHECK_TIMER_MINIMUM_WAIT_TIME;
         }
         MTRWeakReference<MTRDevice *> * weakSelf = [MTRWeakReference weakReferenceWithObject:self];
-        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(waitTime * NSEC_PER_SEC)), _queue, ^{
+        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(waitTime * NSEC_PER_SEC)), self.queue, ^{
             MTRDevice * strongSelf = weakSelf.strongObject;
             [strongSelf _performScheduledExpirationCheck];
         });

src/darwin/Framework/CHIP/MTRDevice_Internal.h

@@ -35,6 +35,10 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);
 
 @property (nonatomic, readonly) MTRDeviceController * deviceController;
 @property (nonatomic, readonly, copy) NSNumber * nodeID;
+// Queue used for various internal bookkeeping work.  In general endWork calls
+// on work items should happen on this queue, so we don't block progress of the
+// asyncCallbackWorkQueue on any client code.
+@property (nonatomic) dispatch_queue_t queue;
 @property (nonatomic, readonly) MTRAsyncCallbackWorkQueue * asyncCallbackWorkQueue;
 
 @end

src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt

@@ -73,12 +73,14 @@ static void MTRClustersLogCompletion(NSString *logPrefix, id value, NSError *err
     if (timedInvokeTimeoutMsParam) {
       timedInvokeTimeoutMsParam = MTRClampedNumber(timedInvokeTimeoutMsParam, @(1), @(UINT16_MAX));
     }
-    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.callbackQueue];
+    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.device.queue];
     MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
         MTRClustersLogDequeue(logPrefix, self.device.asyncCallbackWorkQueue);
         MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.device.nodeID controller:self.device.deviceController];
-        auto * bridge = new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
+        auto * bridge = new MTR{{>callbackName}}CallbackBridge(self.device.queue,
           ^(id _Nullable value, NSError * _Nullable error) {
+            MTRClustersLogCompletion(logPrefix, value, error);
+            dispatch_async(self.callbackQueue, ^{
           {{#if hasSpecificResponse}}
             {{! This treats completion as taking an id for the data.  This is
                 not great from a type-safety perspective, of course. }}
@@ -89,7 +91,7 @@ static void MTRClustersLogCompletion(NSString *logPrefix, id value, NSError *err
                 type-safety perspective, of course. }}
             completion(error);
           {{/if}}
-            MTRClustersLogCompletion(logPrefix, value, error);
+            });
             [workItem endWork];
           },
           ^(ExchangeManager & exchangeManager, const SessionHandle & session, {{>callbackName}}CallbackType successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {