From d38ab9a52956db31e1f35715788cee821b53e620 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Wed, 10 Jul 2024 11:07:07 -0700 Subject: [PATCH] [Darwin] MTRDeviceController getSessionForNode should always use live MTRDevice object (#34269) * [Darwin] MTRDeviceController should always examine the MTRDevice before using subscription/CASE pool * Update src/darwin/Framework/CHIP/MTRDevice.mm Co-authored-by: Justin Wood * Update src/darwin/Framework/CHIP/MTRDeviceController.mm Co-authored-by: Boris Zbarsky --------- Co-authored-by: Justin Wood Co-authored-by: Boris Zbarsky --- src/darwin/Framework/CHIP/MTRDevice.mm | 3 +++ .../Framework/CHIP/MTRDeviceController.mm | 21 +++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 2ae8d5fc310d13..89478554cbdc8b 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -539,6 +539,9 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle - (void)dealloc { [[NSNotificationCenter defaultCenter] removeObserver:_systemTimeChangeObserverToken]; + + // TODO: retain cycle and clean up https://github.com/project-chip/connectedhomeip/issues/34267 + MTR_LOG("MTRDevice dealloc: %p", self); } - (NSString *)description diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index cea281551384d0..643ea407f287df 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -605,11 +605,23 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams MTR_LOG("Loaded attribute values for %lu nodes from storage for controller uuid %@", static_cast(clusterDataByNode.count), self->_uniqueIdentifier); std::lock_guard lock(self->_deviceMapLock); + NSMutableArray * deviceList = [NSMutableArray array]; for (NSNumber * nodeID in clusterDataByNode) { NSDictionary * clusterData = clusterDataByNode[nodeID]; MTRDevice * device = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:clusterData]; MTR_LOG("Loaded %lu cluster data from storage for %@", static_cast(clusterData.count), device); + + [deviceList addObject:device]; } + +#define kSecondsToWaitBeforeAPIClientRetainsMTRDevice 60 + // Keep the devices retained for a while, in case API client doesn't immediately retain them. + // + // Note that this is just an optimization to avoid throwing the information away and immediately + // re-reading it from storage. + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ + MTR_LOG("MTRDeviceController: un-retain devices loaded at startup %lu", static_cast(deviceList.count)); + }); }]; } @@ -1257,15 +1269,12 @@ - (BOOL)checkIsRunning:(NSError * __autoreleasing *)error - (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion { - // 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 objectForKey:@(nodeID)]; - os_unfair_lock_unlock(&_deviceMapLock); + // Get the corresponding MTRDevice object to determine if the case/subscription pool is to be used + MTRDevice * device = [self deviceForNodeID:@(nodeID)]; // In the case that this device is known to use thread, queue this with subscription attempts as well, to // help with throttling Thread traffic. - if (device && [device deviceUsesThread]) { + if ([device deviceUsesThread]) { MTRAsyncWorkItem * workItem = [[MTRAsyncWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; [workItem setReadyHandler:^(id _Nonnull context, NSInteger retryCount, MTRAsyncWorkCompletionBlock _Nonnull workItemCompletion) { MTRInternalDeviceConnectionCallback completionWrapper = ^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,