diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index b84505f8185442..04fa9a9bec05e9 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 9cd3b62340d20a..e47415880cdb13 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -111,7 +111,7 @@ @implementation MTRDeviceControllerFactory { MTRSessionResumptionStorageBridge * _sessionResumptionStorage; PersistentStorageOperationalKeystore * _keystore; Credentials::PersistentStorageOpCertStore * _opCertStore; - MTROperationalBrowser * _operationalBrowser; + std::unique_ptr _operationalBrowser; // productAttestationAuthorityCertificates and certificationDeclarationCertificates are just copied // from MTRDeviceControllerFactoryParams. @@ -223,6 +223,8 @@ - (instancetype)init _serverEndpointsLock = OS_UNFAIR_LOCK_INIT; _serverEndpoints = [[NSMutableArray alloc] init]; + _operationalBrowser = std::make_unique(self, self->_chipWorkQueue); + return self; } @@ -557,12 +559,6 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController * return nil; } - if ([_controllers count] == 0) { - dispatch_sync(_chipWorkQueue, ^{ - self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue); - }); - } - // 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); @@ -943,9 +939,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller // OtaProviderDelegateBridge uses some services provided by the system // state without retaining it. if (_controllers.count == 0) { - delete self->_operationalBrowser; - self->_operationalBrowser = nullptr; - if (_otaProviderDelegateBridge) { _otaProviderDelegateBridge->Shutdown(); _otaProviderDelegateBridge.reset(); @@ -1246,6 +1239,11 @@ - (PersistentStorageDelegate *)storageDelegate return &_groupDataProvider; } +- (MTROperationalBrowser *)operationalBrowser +{ + return _operationalBrowser.get(); +} + @end @interface MTRDummyStorage : NSObject 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 0cdcbf6d869736..4115f0e887d150 100644 --- a/src/darwin/Framework/CHIP/MTROperationalBrowser.h +++ b/src/darwin/Framework/CHIP/MTROperationalBrowser.h @@ -23,19 +23,29 @@ class MTROperationalBrowser { public: - // Should be created at a point when the factory starts up the event loop, - // and destroyed when the event loop is stopped. + // Should be created at a point when the dispatch queue is available. MTROperationalBrowser(MTRDeviceControllerFactory * aFactory, dispatch_queue_t aQueue); ~MTROperationalBrowser(); + // 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 TryToStartBrowse(); + void EnsureBrowse(); + void StopBrowse(); - MTRDeviceControllerFactory * const mDeviceControllerFactory; + MTRDeviceControllerFactory * const __weak mDeviceControllerFactory; dispatch_queue_t mQueue; DNSServiceRef mBrowseRef; @@ -44,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 1ddc7868d88ecc..b5717373ab2933 100644 --- a/src/darwin/Framework/CHIP/MTROperationalBrowser.mm +++ b/src/darwin/Framework/CHIP/MTROperationalBrowser.mm @@ -37,26 +37,49 @@ : mDeviceControllerFactory(aFactory) , mQueue(aQueue) { - // If we fail to start a browse, there's nothing our consumer would do - // differently, so we might as well do this in the constructor. - TryToStartBrowse(); } -void MTROperationalBrowser::TryToStartBrowse() +void MTROperationalBrowser::ControllerActivated() { assertChipStackLockedByCurrentThread(); - ChipLogProgress(Controller, "Trying to start persistent operational browse"); + if (mActiveControllerCount == 0) { + EnsureBrowse(); + } + ++mActiveControllerCount; +} + +void MTROperationalBrowser::ControllerDeactivated() +{ + assertChipStackLockedByCurrentThread(); + + if (mActiveControllerCount == 1) { + StopBrowse(); + } + + --mActiveControllerCount; +} + +void MTROperationalBrowser::EnsureBrowse() +{ + assertChipStackLockedByCurrentThread(); + + if (mInitialized) { + ChipLogProgress(Controller, "%p already has a persistent operational browse running", this); + return; + } + + ChipLogProgress(Controller, "%p trying to start persistent operational browse", this); auto err = DNSServiceCreateConnection(&mBrowseRef); if (err != kDNSServiceErr_NoError) { - ChipLogError(Controller, "Failed to create connection for persistent operational browse: %" PRId32, err); + ChipLogError(Controller, "%p failed to create connection for persistent operational browse: %" PRId32, this, err); return; } err = DNSServiceSetDispatchQueue(mBrowseRef, mQueue); if (err != kDNSServiceErr_NoError) { - ChipLogError(Controller, "Failed to set up dispatch queue properly for persistent operational browse: %" PRId32, err); + ChipLogError(Controller, "%p failed to set up dispatch queue properly for persistent operational browse: %" PRId32, this, err); DNSServiceRefDeallocate(mBrowseRef); return; } @@ -67,11 +90,20 @@ auto browseRef = mBrowseRef; // Mandatory copy because of kDNSServiceFlagsShareConnection. err = DNSServiceBrowse(&browseRef, kDNSServiceFlagsShareConnection, kDNSServiceInterfaceIndexAny, kOperationalType, domain, OnBrowse, this); if (err != kDNSServiceErr_NoError) { - ChipLogError(Controller, "Failed to start persistent operational browse for \"%s\" domain: %" PRId32, StringOrNullMarker(domain), err); + ChipLogError(Controller, "%p failed to start persistent operational browse for \"%s\" domain: %" PRId32, this, StringOrNullMarker(domain), err); } } } +void MTROperationalBrowser::StopBrowse() +{ + ChipLogProgress(Controller, "%p stopping persistent operational browse", this); + if (mInitialized) { + DNSServiceRefDeallocate(mBrowseRef); + mInitialized = false; + } +} + void MTROperationalBrowser::OnBrowse(DNSServiceRef aServiceRef, DNSServiceFlags aFlags, uint32_t aInterfaceId, DNSServiceErrorType aError, const char * aName, const char * aType, const char * aDomain, void * aContext) { @@ -82,14 +114,13 @@ // We only expect to get notified about our type/domain. if (aError != kDNSServiceErr_NoError) { ChipLogError(Controller, "Operational browse failure: %" PRId32, aError); - DNSServiceRefDeallocate(self->mBrowseRef); - self->mInitialized = false; + self->StopBrowse(); // We shouldn't really get callbacks under our destructor, but guard // against it just in case. if (!self->mIsDestroying) { // Try to start a new browse, so we have one going. - self->TryToStartBrowse(); + self->EnsureBrowse(); } return; } @@ -116,7 +147,5 @@ mIsDestroying = true; - if (mInitialized) { - DNSServiceRefDeallocate(mBrowseRef); - } + StopBrowse(); } diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 48500f2933dcb4..db0a8ab848576c 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -76,6 +76,30 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr @end +@interface MTRPerControllerStorageTestsSuspensionDelegate : NSObject +@property (nonatomic, strong) XCTestExpectation * expectation; +@property (nonatomic, assign) BOOL expectedSuspensionState; +@end + +@implementation MTRPerControllerStorageTestsSuspensionDelegate +- (id)initWithExpectation:(XCTestExpectation *)expectation expectedSuspensionState:(BOOL)expectedSuspensionState +{ + self = [super init]; + if (self) { + _expectation = expectation; + _expectedSuspensionState = expectedSuspensionState; + } + return self; +} + +- (void)controller:(MTRDeviceController *)controller + isSuspended:(BOOL)suspended +{ + XCTAssertEqual(suspended, self.expectedSuspensionState); + [self.expectation fulfill]; +} +@end + @interface MTRPerControllerStorageTestsCertificateIssuer : NSObject - (instancetype)initWithRootCertificate:(MTRCertificateDERBytes)rootCertificate intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate @@ -1713,6 +1737,17 @@ - (void)test013_suspendDevices [becameUnreachableExpectation fulfill]; }; + XCTestExpectation * suspendedExpectation = [self expectationWithDescription:@"Controller has been suspended"]; + __auto_type * suspensionDelegate = [[MTRPerControllerStorageTestsSuspensionDelegate alloc] initWithExpectation:suspendedExpectation expectedSuspensionState:YES]; + [controller addDeviceControllerDelegate:suspensionDelegate queue:queue]; + + XCTestExpectation * browseStoppedExpectation = [self expectationWithDescription:@"Operational browse has stopped"]; + MTRSetLogCallback(MTRLogTypeProgress, ^(MTRLogType type, NSString * moduleName, NSString * message) { + if ([message containsString:@"stopping persistent operational browse"]) { + [browseStoppedExpectation fulfill]; + } + }); + [controller suspend]; XCTAssertTrue(controller.suspended); @@ -1723,7 +1758,7 @@ - (void)test013_suspendDevices [toggle2Expectation fulfill]; }]; - [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation ] timeout:kTimeoutInSeconds]; + [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation ] timeout:kTimeoutInSeconds]; XCTestExpectation * newSubscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up again"]; XCTestExpectation * newReachableExpectation = [self expectationWithDescription:@"Device became reachable again"]; @@ -1735,10 +1770,23 @@ - (void)test013_suspendDevices [newSubscriptionExpectation fulfill]; }; + XCTestExpectation * resumedExpectation = [self expectationWithDescription:@"Controller has been resumed"]; + suspensionDelegate.expectation = resumedExpectation; + suspensionDelegate.expectedSuspensionState = NO; + + XCTestExpectation * browseRestartedExpectation = [self expectationWithDescription:@"Operational browse has re-started"]; + MTRSetLogCallback(MTRLogTypeProgress, ^(MTRLogType type, NSString * moduleName, NSString * message) { + if ([message containsString:@"trying to start persistent operational browse"]) { + [browseRestartedExpectation fulfill]; + } + }); + [controller resume]; XCTAssertFalse(controller.suspended); - [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation ] timeout:kSubscriptionTimeoutInSeconds]; + [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation ] timeout:kSubscriptionTimeoutInSeconds]; + + MTRSetLogCallback(MTRLogTypeProgress, nil); // Test that sending a command works again. Clear the delegate's onReportEnd // first, so reports from the command don't trigger it.