From 1227527a8cc770f4b0b6a2e8c1ffd00848782b15 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 16:11:59 -0400 Subject: [PATCH] Remove shutdown implementation from base MTRDeviceController. (#35676) shutdown is overriden by both MTRDeviceController_Concrete and MTRDeviceController_XPC, so the base class implementation is not reachable. And it's the only caller of finalShutdown, so that can also be removed. Also, addRunAssertion/removeRunAssertion, are only used on concrete controllers and can be removed from the base class and from MTRDeviceController_Internal. With those removed, matchesPendingShutdownControllerWithOperationalCertificate would always return false, so that can be changed accordingly, and then clearPendingShutdown becomes unreachable. At this point _keepRunningAssertionCounter and _shutdownPending are never read and can be removed. And _assertionLock is never acquired, so it can also be removed. --- .../Framework/CHIP/MTRDeviceController.mm | 85 ++----------------- .../CHIP/MTRDeviceControllerFactory.mm | 3 + .../CHIP/MTRDeviceController_Concrete.h | 15 ++++ .../CHIP/MTRDeviceController_Internal.h | 14 --- 4 files changed, 26 insertions(+), 91 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index e39a4d14e75612..4c3afe8412040b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -26,6 +26,7 @@ #import "MTRCommissionableBrowserResult_Internal.h" #import "MTRCommissioningParameters.h" #import "MTRConversion.h" +#import "MTRDefines_Internal.h" #import "MTRDeviceControllerDelegateBridge.h" #import "MTRDeviceControllerFactory_Internal.h" #import "MTRDeviceControllerLocalTestStorage.h" @@ -160,11 +161,6 @@ @implementation MTRDeviceController { // specific queue, so can't race against each other. std::atomic _suspended; - // Counters to track assertion status and access controlled by the _assertionLock - NSUInteger _keepRunningAssertionCounter; - BOOL _shutdownPending; - os_unfair_lock _assertionLock; - NSMutableArray * _delegates; id _strongDelegateForSetDelegateAPI; } @@ -183,11 +179,6 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended } _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT; - // Setup assertion variables - _keepRunningAssertionCounter = 0; - _shutdownPending = NO; - _assertionLock = OS_UNFAIR_LOCK_INIT; - _suspended = startSuspended; _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; @@ -231,11 +222,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory // before we start doing anything else with the controller. _uniqueIdentifier = uniqueIdentifier; - // Setup assertion variables - _keepRunningAssertionCounter = 0; - _shutdownPending = NO; - _assertionLock = OS_UNFAIR_LOCK_INIT; - _suspended = startSuspended; if (storageDelegate != nil) { @@ -478,75 +464,21 @@ - (void)_controllerResumed - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate { - if (!operationalCertificate || !rootCertificate) { - return FALSE; - } - NSNumber * nodeID = [MTRDeviceControllerParameters nodeIDFromNOC:operationalCertificate]; - NSNumber * fabricID = [MTRDeviceControllerParameters fabricIDFromNOC:operationalCertificate]; - NSData * publicKey = [MTRDeviceControllerParameters publicKeyFromCertificate:rootCertificate]; - - std::lock_guard lock(_assertionLock); - - // If any of the local above are nil, the return will be false since MTREqualObjects handles them correctly - return _keepRunningAssertionCounter > 0 && _shutdownPending && MTREqualObjects(nodeID, self.nodeID) && MTREqualObjects(fabricID, self.fabricID) && MTREqualObjects(publicKey, self.rootPublicKey); -} - -- (void)addRunAssertion -{ - std::lock_guard lock(_assertionLock); - - // Only take an assertion if running - if ([self isRunning]) { - ++_keepRunningAssertionCounter; - MTR_LOG("%@ Adding keep running assertion, total %lu", self, static_cast(_keepRunningAssertionCounter)); - } -} - -- (void)removeRunAssertion; -{ - std::lock_guard lock(_assertionLock); - - if (_keepRunningAssertionCounter > 0) { - --_keepRunningAssertionCounter; - MTR_LOG("%@ Removing keep running assertion, total %lu", self, static_cast(_keepRunningAssertionCounter)); - - if ([self isRunning] && _keepRunningAssertionCounter == 0 && _shutdownPending) { - MTR_LOG("%@ All assertions removed and shutdown is pending, shutting down", self); - [self finalShutdown]; - } - } + // TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its + // declaration moved to MTRDeviceController_Concrete. + return NO; } - (void)clearPendingShutdown { - std::lock_guard lock(_assertionLock); - _shutdownPending = NO; + // TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its + // declaration moved to MTRDeviceController_Concrete. + MTR_ABSTRACT_METHOD(); } - (void)shutdown { - std::lock_guard lock(_assertionLock); - - if (_keepRunningAssertionCounter > 0) { - MTR_LOG("%@ Pending shutdown since %lu assertions are present", self, static_cast(_keepRunningAssertionCounter)); - _shutdownPending = YES; - return; - } - [self finalShutdown]; -} - -- (void)finalShutdown -{ - os_unfair_lock_assert_owner(&_assertionLock); - - MTR_LOG("%@ shutdown called", self); - if (_cppCommissioner == nullptr) { - // Already shut down. - return; - } - - MTR_LOG("Shutting down %@: %@", NSStringFromClass(self.class), self); - [self cleanupAfterStartup]; + MTR_ABSTRACT_METHOD(); } // Clean up from a state where startup was called. @@ -604,7 +536,6 @@ - (void)shutDownCppController _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); } } - _shutdownPending = NO; } - (void)deinitFromFactory diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index ee0f330b26cfc2..c44b6e3bfb25dc 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1144,6 +1144,9 @@ - (nullable MTRDeviceController *)_findPendingShutdownControllerWithOperationalC { std::lock_guard lock(_controllersLock); for (MTRDeviceController * controller in _controllers) { + // TODO: Once we know our controllers are MTRDeviceController_Concrete, move + // matchesPendingShutdownControllerWithOperationalCertificate and clearPendingShutdown to that + // interface and remove them from base MTRDeviceController_Internal. if ([controller matchesPendingShutdownControllerWithOperationalCertificate:operationalCertificate andRootCertificate:rootCertificate]) { MTR_LOG("%@ Found existing controller %@ that is pending shutdown and matching parameters, re-using it", self, controller); [controller clearPendingShutdown]; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index b491e3f140c85a..59dd42c04a97f4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -23,6 +23,21 @@ NS_ASSUME_NONNULL_BEGIN @interface MTRDeviceController_Concrete : MTRDeviceController + +/** + * 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 + * the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release + * the assertion. + */ +- (void)addRunAssertion; + +/** + * Removes an assertion to allow the controller to shutdown once all assertions have been released. + * Invoking this method once all assertions have been released in a noop. + */ +- (void)removeRunAssertion; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 993298234f33ef..8eb21aef64eb9e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -308,20 +308,6 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion; -/** - * 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 - * the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release - * the assertion. - */ -- (void)addRunAssertion; - -/** - * Removes an assertion to allow the controller to shutdown once all assertions have been released. - * Invoking this method once all assertions have been released in a noop. - */ -- (void)removeRunAssertion; - /** * This method returns TRUE if this controller matches the fabric reference and node ID as listed in the parameters. */