From edb40140968c83a65d3674769ab317b51020e90b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 17:06:00 -0400 Subject: [PATCH] Remove unreachable shutdown code from MTRDeviceController. cleanupAfterStartup is no longer reachable and can be removed. At that point, the only caller of controllerShuttingDown: on the factory is MTRDeviceController_Concrete, so we can go ahead and change the argument type accordingly. At that point it becomes obvious that shutDownCppController and deinitFromFactory are only called on MTRDeviceController_Concrete instances, so those can move from MTRDeviceController_Internal to MTRDeviceController_Concrete, and the implementations in MTRDeviceController can be removed. checkForInitError is now unused on MTRDeviceController, and can be removed. And then "cleanup" is unused and can be removed. --- .../Framework/CHIP/MTRDeviceController.mm | 112 ------------------ .../CHIP/MTRDeviceControllerFactory.mm | 2 +- .../MTRDeviceControllerFactory_Internal.h | 2 +- .../CHIP/MTRDeviceController_Concrete.h | 17 +++ .../CHIP/MTRDeviceController_Internal.h | 17 --- 5 files changed, 19 insertions(+), 131 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index b8e454b49080c3..e069ea79a990c8 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -339,105 +339,6 @@ - (void)shutdown MTR_ABSTRACT_METHOD(); } -// Clean up from a state where startup was called. -- (void)cleanupAfterStartup -{ - // Invalidate our MTRDevice instances before we shut down our secure - // sessions and whatnot, so they don't start trying to resubscribe when we - // do the secure session shutdowns. Since we don't want to hold the lock - // while calling out into arbitrary invalidation code, snapshot the list of - // devices before we start invalidating. - MTR_LOG("%s: %@", __PRETTY_FUNCTION__, self); - os_unfair_lock_lock(self.deviceMapLock); - auto * devices = [self.nodeIDToDeviceMap objectEnumerator].allObjects; - [_nodeIDToDeviceMap removeAllObjects]; - os_unfair_lock_unlock(self.deviceMapLock); - - for (MTRDevice * device in devices) { - [device invalidate]; - } - [self stopBrowseForCommissionables]; - - [_factory controllerShuttingDown:self]; -} - -// Part of cleanupAfterStartup that has to interact with the Matter work queue -// in a very specific way that only MTRDeviceControllerFactory knows about. -- (void)shutDownCppController -{ - MTR_LOG("%s: %p", __PRETTY_FUNCTION__, self); - assertChipStackLockedByCurrentThread(); - - // Shut down all our endpoints. - for (MTRServerEndpoint * endpoint in [_serverEndpoints copy]) { - [self removeServerEndpointOnMatterQueue:endpoint]; - } - - if (_cppCommissioner) { - auto * commissionerToShutDown = _cppCommissioner; - // Flag ourselves as not running before we start shutting down - // _cppCommissioner, so we're not in a state where we claim to be - // running but are actually partially shut down. - _cppCommissioner = nullptr; - commissionerToShutDown->Shutdown(); - // Don't clear out our fabric index association until controller - // shutdown completes, in case it wants to write to storage as it - // shuts down. - _storedFabricIndex = chip::kUndefinedFabricIndex; - _storedCompressedFabricID = std::nullopt; - self.nodeID = nil; - self.fabricID = nil; - self.rootPublicKey = nil; - - delete commissionerToShutDown; - if (_operationalCredentialsDelegate != nil) { - _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); - } - } -} - -- (void)deinitFromFactory -{ - [self cleanup]; -} - -// Clean up any members we might have allocated. -- (void)cleanup -{ - VerifyOrDie(_cppCommissioner == nullptr); - - if (_defaultDACVerifier) { - delete _defaultDACVerifier; - _defaultDACVerifier = nullptr; - } - - if (_attestationTrustStoreBridge) { - delete _attestationTrustStoreBridge; - _attestationTrustStoreBridge = nullptr; - } - - [self clearDeviceAttestationDelegateBridge]; - - if (_operationalCredentialsDelegate) { - delete _operationalCredentialsDelegate; - _operationalCredentialsDelegate = nullptr; - } - - if (_partialDACVerifier) { - delete _partialDACVerifier; - _partialDACVerifier = nullptr; - } - - if (_deviceControllerDelegateBridge) { - delete _deviceControllerDelegateBridge; - _deviceControllerDelegateBridge = nullptr; - @synchronized(self) { - _strongDelegateForSetDelegateAPI = nil; - [_delegates removeAllObjects]; - } - } -} - - (NSNumber *)controllerNodeID { auto block = ^NSNumber * { return @(self->_cppCommissioner->GetNodeId()); }; @@ -968,19 +869,6 @@ - (void)removeServerEndpointOnMatterQueue:(MTRServerEndpoint *)endpoint [_factory removeServerEndpoint:endpoint]; } -- (BOOL)checkForInitError:(BOOL)condition logMsg:(NSString *)logMsg -{ - if (condition) { - return NO; - } - - MTR_LOG_ERROR("%@ Error: %@", self, logMsg); - - [self cleanup]; - - return YES; -} - - (void)clearDeviceAttestationDelegateBridge { if (_deviceAttestationDelegateBridge) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index d052eef51d0414..2db245545bc8f9 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -906,7 +906,7 @@ - (void)resetOperationalAdvertising app::DnssdServer::Instance().StartServer(); } -- (void)controllerShuttingDown:(MTRDeviceController *)controller +- (void)controllerShuttingDown:(MTRDeviceController_Concrete *)controller { [self _assertCurrentQueueIsNotMatterQueue]; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index 55c67ed8043c70..94c6fe422355ae 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -51,7 +51,7 @@ NS_ASSUME_NONNULL_BEGIN MTR_DIRECT_MEMBERS @interface MTRDeviceControllerFactory () -- (void)controllerShuttingDown:(MTRDeviceController *)controller; +- (void)controllerShuttingDown:(MTRDeviceController_Concrete *)controller; /** * Get the list of running controllers. This will include controllers that are diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index 615765fc84dae1..c33cd314d00361 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -54,6 +54,23 @@ NS_ASSUME_NONNULL_BEGIN */ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams; +/** + * Shut down the underlying C++ controller. Must be called on the Matter work + * queue or after the Matter work queue has been shut down. + * + * Only MTRDeviceControllerFactory should be calling this. + */ +- (void)shutDownCppController; + +/** + * Notification that the MTRDeviceControllerFactory has finished shutting down + * this controller and will not be touching it anymore. This is guaranteed to + * be called after initWithFactory succeeds. + * + * Only MTRDeviceControllerFactory should be calling this. + */ +- (void)deinitFromFactory; + /** * Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion * is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index bcdcb70d10cfc2..182eb609f06820 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -136,23 +136,6 @@ NS_ASSUME_NONNULL_BEGIN fabricIndex:(chip::FabricIndex)fabricIndex isRunning:(BOOL *)isRunning; -/** - * Shut down the underlying C++ controller. Must be called on the Matter work - * queue or after the Matter work queue has been shut down. - * - * Only MTRDeviceControllerFactory should be calling this. - */ -- (void)shutDownCppController; - -/** - * Notification that the MTRDeviceControllerFactory has finished shutting down - * this controller and will not be touching it anymore. This is guaranteed to - * be called after initWithFactory succeeds. - * - * Only MTRDeviceControllerFactory should be calling this. - */ -- (void)deinitFromFactory; - /** * Ensure we have a CASE session to the given node ID and then call the provided * connection callback. This may be called on any queue (including the Matter