Remove shutdown implementation from base MTRDeviceController.

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.
bzbarsky-apple · Sep 19, 2024 · 8f896ab · 8f896ab
1 parent 12f04dc
commit 8f896ab
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 91 deletions.
diff --git 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<bool> _suspended;
 
-    // Counters to track assertion status and access controlled by the _assertionLock
-    NSUInteger _keepRunningAssertionCounter;
-    BOOL _shutdownPending;
-    os_unfair_lock _assertionLock;
-
     NSMutableArray<MTRDeviceControllerDelegateInfo *> * _delegates;
     id<MTRDeviceControllerDelegate> _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<unsigned long>(_keepRunningAssertionCounter));
-    }
-}
-
-- (void)removeRunAssertion;
-{
-    std::lock_guard lock(_assertionLock);
-
-    if (_keepRunningAssertionCounter > 0) {
-        --_keepRunningAssertionCounter;
-        MTR_LOG("%@ Removing keep running assertion, total %lu", self, static_cast<unsigned long>(_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<unsigned long>(_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
@@ -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
@@ -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
@@ -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.
  */