From 36d8668b929768d4335e7177e2b4ccf6d246d9be Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 12 Sep 2024 11:24:50 -0400 Subject: [PATCH] Address review comments. --- .../Framework/CHIP/MTRDeviceController.mm | 38 ++++++++++-- .../CHIP/MTRDeviceControllerFactory.mm | 58 +------------------ .../MTRDeviceControllerFactory_Internal.h | 2 + .../CHIP/MTRDeviceController_Concrete.mm | 30 ++++++++++ .../Framework/CHIP/MTROperationalBrowser.h | 19 +++++- .../Framework/CHIP/MTROperationalBrowser.mm | 26 +++++++-- 6 files changed, 107 insertions(+), 66 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index dc36aeb694d1e7..6fa84372884db4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -188,10 +188,6 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended _shutdownPending = NO; _assertionLock = OS_UNFAIR_LOCK_INIT; - // All synchronous suspend/resume activity has to be protected by - // @synchronized(self), so that parts of suspend/resume can't - // interleave with each other. Using @synchronized here because - // MTRDevice may call isSuspended. _suspended = startSuspended; _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; @@ -399,6 +395,11 @@ - (void)suspend { MTR_LOG("%@ suspending", self); + if (![self isRunning]) { + MTR_LOG_ERROR("%@ not running; can't suspend", self); + return; + } + NSArray * devicesToSuspend; { std::lock_guard lock(*self.deviceMapLock); @@ -406,6 +407,11 @@ - (void)suspend // for any given device exactly one of two things is true: // * It is in the snapshot we are creating // * It is created after we have changed our _suspended state. + if (_suspended) { + MTR_LOG("%@ already suspended", self); + return; + } + _suspended = YES; devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects; } @@ -421,12 +427,24 @@ - (void)suspend // * CASE sessions in general. // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising. [self _notifyDelegatesOfSuspendState]; + + [self _controllerSuspended]; +} + +- (void)_controllerSuspended +{ + // Subclass hook; nothing to do. } - (void)resume { MTR_LOG("%@ resuming", self); + if (![self isRunning]) { + MTR_LOG_ERROR("%@ not running; can't resume", self); + return; + } + NSArray * devicesToResume; { std::lock_guard lock(*self.deviceMapLock); @@ -434,6 +452,11 @@ - (void)resume // for any given device exactly one of two things is true: // * It is in the snapshot we are creating // * It is created after we have changed our _suspended state. + if (!_suspended) { + MTR_LOG("%@ already not suspended", self); + return; + } + _suspended = NO; devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects; } @@ -444,6 +467,13 @@ - (void)resume } [self _notifyDelegatesOfSuspendState]; + + [self _controllerResumed]; +} + +- (void)_controllerResumed +{ + // Subclass hook; nothing to do. } - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 1a4811a3b2a7ba..e47415880cdb13 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -86,7 +86,7 @@ @interface MTRDeviceControllerFactoryParams () @end MTR_DIRECT_MEMBERS -@interface MTRDeviceControllerFactory () +@interface MTRDeviceControllerFactory () - (void)preWarmCommissioningSessionDone; @end @@ -559,21 +559,6 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController * return nil; } - // Listen for changes to suspended state on this controller, so we can - // track whether we have any unsuspended controllers. - [controller addDeviceControllerDelegate:self queue:_chipWorkQueue]; - - if (!startSuspended) { - // Async dispatch here is fine: in the unlikely case that someone comes - // along and shuts down or suspends controllers so that we have to stop - // the browse, and our async dispatch runs after that, we'll just - // briefly have a browse that we don't really need, then end up shutting - // it down again. That's what would happen with a sync dispatch anyway. - dispatch_async(_chipWorkQueue, ^{ - self->_operationalBrowser->EnsureBrowse(); - }); - } - // Add the controller to _controllers now, so if we fail partway through its // startup we will still do the right cleanups. os_unfair_lock_lock(&_controllersLock); @@ -949,12 +934,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller self->_groupDataProvider.RemoveGroupKeys(fabricIndex); } - // If all remaining controllers after this one has been removed are - // suspended, we should stop our operational browse. - if ([self _allControllersSuspended]) { - self->_operationalBrowser->StopBrowse(); - } - // If there are no other controllers left, we can shut down some things. // Do this before we shut down the controller itself, because the // OtaProviderDelegateBridge uses some services provided by the system @@ -1260,40 +1239,9 @@ - (PersistentStorageDelegate *)storageDelegate return &_groupDataProvider; } -#pragma mark - Controller suspension support - -- (BOOL)_allControllersSuspended +- (MTROperationalBrowser *)operationalBrowser { - auto * controllers = [self getRunningControllers]; - for (MTRDeviceController * controller in controllers) { - if (!controller.suspended) { - return NO; - } - } - return YES; -} - -- (void)controller:(MTRDeviceController *)controller - isSuspended:(BOOL)suspended -{ - // We use the Matter queue as our delegate queue for this notification. - assertChipStackLockedByCurrentThread(); - - if (!suspended) { - // As long as we have one controller that is not suspended, we should - // have a running operational browse. Guard against someone resuming a - // controller after it has been shut down (at which point it's not in - // our "running controllers" list). - auto * controllers = [self getRunningControllers]; - if ([controllers containsObject:controller]) { - _operationalBrowser->EnsureBrowse(); - } - return; - } - - if ([self _allControllersSuspended]) { - _operationalBrowser->StopBrowse(); - } + return _operationalBrowser.get(); } @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index c35a0b4a1692f0..58592b6ed4b670 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -30,6 +30,7 @@ #import "MTRDefines_Internal.h" #import "MTRDeviceControllerFactory.h" +#import "MTROperationalBrowser.h" #include #include @@ -108,6 +109,7 @@ MTR_DIRECT_MEMBERS @property (readonly) chip::PersistentStorageDelegate * storageDelegate; @property (readonly) chip::Credentials::GroupDataProvider * groupDataProvider; +@property (readonly, assign) MTROperationalBrowser * operationalBrowser; @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index bdcbafae1b6c9d..a73a644b85302a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -330,6 +330,15 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory self.rootPublicKey = nil; _storageBehaviorConfiguration = storageBehaviorConfiguration; + + // We let the operational browser know about ourselves here, because + // after this point we are guaranteed to have shutDownCppController + // called by the factory. + if (!startSuspended) { + dispatch_async(_chipWorkQueue, ^{ + factory.operationalBrowser->ControllerActivated(); + }); + } } return self; } @@ -344,6 +353,22 @@ - (BOOL)isRunning return _cppCommissioner != nullptr; } +- (void)_controllerSuspended +{ + MTRDeviceControllerFactory * factory = _factory; + dispatch_async(_chipWorkQueue, ^{ + factory.operationalBrowser->ControllerDeactivated(); + }); +} + +- (void)_controllerResumed +{ + MTRDeviceControllerFactory * factory = _factory; + dispatch_async(_chipWorkQueue, ^{ + factory.operationalBrowser->ControllerActivated(); + }); +} + - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate { if (!operationalCertificate || !rootCertificate) { @@ -471,6 +496,11 @@ - (void)shutDownCppController _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); } } + + if (!self.suspended) { + _factory.operationalBrowser->ControllerDeactivated(); + } + _shutdownPending = NO; } diff --git a/src/darwin/Framework/CHIP/MTROperationalBrowser.h b/src/darwin/Framework/CHIP/MTROperationalBrowser.h index 7d55edf5b6cfaf..4115f0e887d150 100644 --- a/src/darwin/Framework/CHIP/MTROperationalBrowser.h +++ b/src/darwin/Framework/CHIP/MTROperationalBrowser.h @@ -28,14 +28,23 @@ class MTROperationalBrowser ~MTROperationalBrowser(); - // EnsureBrowse is a no-op if a browse has already been started. - void EnsureBrowse(); - void StopBrowse(); + // ControllerActivated should be called, on the Matter queue, when a + // controller is either started in a non-suspended state or stops being + // suspended. + + // ControllerDeactivated should be called, on the Matter queue, when a + // controller is either suspended or shut down while in a non-suspended + // state. + void ControllerActivated(); + void ControllerDeactivated(); private: static void OnBrowse(DNSServiceRef aServiceRef, DNSServiceFlags aFlags, uint32_t aInterfaceId, DNSServiceErrorType aError, const char * aName, const char * aType, const char * aDomain, void * aContext); + void EnsureBrowse(); + void StopBrowse(); + MTRDeviceControllerFactory * const __weak mDeviceControllerFactory; dispatch_queue_t mQueue; DNSServiceRef mBrowseRef; @@ -45,4 +54,8 @@ class MTROperationalBrowser // If mIsDestroying is true, we're in our destructor, shutting things down. bool mIsDestroying = false; + + // Count of controllers that are currently active; we aim to have a browse + // going while this is nonzero; + size_t mActiveControllerCount = 0; }; diff --git a/src/darwin/Framework/CHIP/MTROperationalBrowser.mm b/src/darwin/Framework/CHIP/MTROperationalBrowser.mm index 37dd683b2342ad..b5717373ab2933 100644 --- a/src/darwin/Framework/CHIP/MTROperationalBrowser.mm +++ b/src/darwin/Framework/CHIP/MTROperationalBrowser.mm @@ -39,6 +39,27 @@ { } +void MTROperationalBrowser::ControllerActivated() +{ + assertChipStackLockedByCurrentThread(); + + if (mActiveControllerCount == 0) { + EnsureBrowse(); + } + ++mActiveControllerCount; +} + +void MTROperationalBrowser::ControllerDeactivated() +{ + assertChipStackLockedByCurrentThread(); + + if (mActiveControllerCount == 1) { + StopBrowse(); + } + + --mActiveControllerCount; +} + void MTROperationalBrowser::EnsureBrowse() { assertChipStackLockedByCurrentThread(); @@ -117,10 +138,7 @@ } ChipLogProgress(Controller, "Notifying controller factory about new operational instance: '%s'", aName); - MTRDeviceControllerFactory * strongFactory = self->mDeviceControllerFactory; - if (strongFactory) { - [strongFactory operationalInstanceAdded:peerId]; - } + [self->mDeviceControllerFactory operationalInstanceAdded:peerId]; } MTROperationalBrowser::~MTROperationalBrowser()