Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 8f896ab

Browse files
committedSep 19, 2024·
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.
1 parent 12f04dc commit 8f896ab

4 files changed

+26
-91
lines changed
 

‎src/darwin/Framework/CHIP/MTRDeviceController.mm

+8-77
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#import "MTRCommissionableBrowserResult_Internal.h"
2727
#import "MTRCommissioningParameters.h"
2828
#import "MTRConversion.h"
29+
#import "MTRDefines_Internal.h"
2930
#import "MTRDeviceControllerDelegateBridge.h"
3031
#import "MTRDeviceControllerFactory_Internal.h"
3132
#import "MTRDeviceControllerLocalTestStorage.h"
@@ -160,11 +161,6 @@ @implementation MTRDeviceController {
160161
// specific queue, so can't race against each other.
161162
std::atomic<bool> _suspended;
162163

163-
// Counters to track assertion status and access controlled by the _assertionLock
164-
NSUInteger _keepRunningAssertionCounter;
165-
BOOL _shutdownPending;
166-
os_unfair_lock _assertionLock;
167-
168164
NSMutableArray<MTRDeviceControllerDelegateInfo *> * _delegates;
169165
id<MTRDeviceControllerDelegate> _strongDelegateForSetDelegateAPI;
170166
}
@@ -183,11 +179,6 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
183179
}
184180
_underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT;
185181

186-
// Setup assertion variables
187-
_keepRunningAssertionCounter = 0;
188-
_shutdownPending = NO;
189-
_assertionLock = OS_UNFAIR_LOCK_INIT;
190-
191182
_suspended = startSuspended;
192183

193184
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
@@ -231,11 +222,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
231222
// before we start doing anything else with the controller.
232223
_uniqueIdentifier = uniqueIdentifier;
233224

234-
// Setup assertion variables
235-
_keepRunningAssertionCounter = 0;
236-
_shutdownPending = NO;
237-
_assertionLock = OS_UNFAIR_LOCK_INIT;
238-
239225
_suspended = startSuspended;
240226

241227
if (storageDelegate != nil) {
@@ -478,75 +464,21 @@ - (void)_controllerResumed
478464

479465
- (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate
480466
{
481-
if (!operationalCertificate || !rootCertificate) {
482-
return FALSE;
483-
}
484-
NSNumber * nodeID = [MTRDeviceControllerParameters nodeIDFromNOC:operationalCertificate];
485-
NSNumber * fabricID = [MTRDeviceControllerParameters fabricIDFromNOC:operationalCertificate];
486-
NSData * publicKey = [MTRDeviceControllerParameters publicKeyFromCertificate:rootCertificate];
487-
488-
std::lock_guard lock(_assertionLock);
489-
490-
// If any of the local above are nil, the return will be false since MTREqualObjects handles them correctly
491-
return _keepRunningAssertionCounter > 0 && _shutdownPending && MTREqualObjects(nodeID, self.nodeID) && MTREqualObjects(fabricID, self.fabricID) && MTREqualObjects(publicKey, self.rootPublicKey);
492-
}
493-
494-
- (void)addRunAssertion
495-
{
496-
std::lock_guard lock(_assertionLock);
497-
498-
// Only take an assertion if running
499-
if ([self isRunning]) {
500-
++_keepRunningAssertionCounter;
501-
MTR_LOG("%@ Adding keep running assertion, total %lu", self, static_cast<unsigned long>(_keepRunningAssertionCounter));
502-
}
503-
}
504-
505-
- (void)removeRunAssertion;
506-
{
507-
std::lock_guard lock(_assertionLock);
508-
509-
if (_keepRunningAssertionCounter > 0) {
510-
--_keepRunningAssertionCounter;
511-
MTR_LOG("%@ Removing keep running assertion, total %lu", self, static_cast<unsigned long>(_keepRunningAssertionCounter));
512-
513-
if ([self isRunning] && _keepRunningAssertionCounter == 0 && _shutdownPending) {
514-
MTR_LOG("%@ All assertions removed and shutdown is pending, shutting down", self);
515-
[self finalShutdown];
516-
}
517-
}
467+
// TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its
468+
// declaration moved to MTRDeviceController_Concrete.
469+
return NO;
518470
}
519471

520472
- (void)clearPendingShutdown
521473
{
522-
std::lock_guard lock(_assertionLock);
523-
_shutdownPending = NO;
474+
// TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its
475+
// declaration moved to MTRDeviceController_Concrete.
476+
MTR_ABSTRACT_METHOD();
524477
}
525478

526479
- (void)shutdown
527480
{
528-
std::lock_guard lock(_assertionLock);
529-
530-
if (_keepRunningAssertionCounter > 0) {
531-
MTR_LOG("%@ Pending shutdown since %lu assertions are present", self, static_cast<unsigned long>(_keepRunningAssertionCounter));
532-
_shutdownPending = YES;
533-
return;
534-
}
535-
[self finalShutdown];
536-
}
537-
538-
- (void)finalShutdown
539-
{
540-
os_unfair_lock_assert_owner(&_assertionLock);
541-
542-
MTR_LOG("%@ shutdown called", self);
543-
if (_cppCommissioner == nullptr) {
544-
// Already shut down.
545-
return;
546-
}
547-
548-
MTR_LOG("Shutting down %@: %@", NSStringFromClass(self.class), self);
549-
[self cleanupAfterStartup];
481+
MTR_ABSTRACT_METHOD();
550482
}
551483

552484
// Clean up from a state where startup was called.
@@ -604,7 +536,6 @@ - (void)shutDownCppController
604536
_operationalCredentialsDelegate->SetDeviceCommissioner(nullptr);
605537
}
606538
}
607-
_shutdownPending = NO;
608539
}
609540

610541
- (void)deinitFromFactory

‎src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

+3
Original file line numberDiff line numberDiff line change
@@ -1144,6 +1144,9 @@ - (nullable MTRDeviceController *)_findPendingShutdownControllerWithOperationalC
11441144
{
11451145
std::lock_guard lock(_controllersLock);
11461146
for (MTRDeviceController * controller in _controllers) {
1147+
// TODO: Once we know our controllers are MTRDeviceController_Concrete, move
1148+
// matchesPendingShutdownControllerWithOperationalCertificate and clearPendingShutdown to that
1149+
// interface and remove them from base MTRDeviceController_Internal.
11471150
if ([controller matchesPendingShutdownControllerWithOperationalCertificate:operationalCertificate andRootCertificate:rootCertificate]) {
11481151
MTR_LOG("%@ Found existing controller %@ that is pending shutdown and matching parameters, re-using it", self, controller);
11491152
[controller clearPendingShutdown];

‎src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h

+15
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,21 @@
2323
NS_ASSUME_NONNULL_BEGIN
2424

2525
@interface MTRDeviceController_Concrete : MTRDeviceController
26+
27+
/**
28+
* Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion
29+
* is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases
30+
* the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release
31+
* the assertion.
32+
*/
33+
- (void)addRunAssertion;
34+
35+
/**
36+
* Removes an assertion to allow the controller to shutdown once all assertions have been released.
37+
* Invoking this method once all assertions have been released in a noop.
38+
*/
39+
- (void)removeRunAssertion;
40+
2641
@end
2742

2843
NS_ASSUME_NONNULL_END

‎src/darwin/Framework/CHIP/MTRDeviceController_Internal.h

-14
Original file line numberDiff line numberDiff line change
@@ -308,20 +308,6 @@ NS_ASSUME_NONNULL_BEGIN
308308
*/
309309
- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion;
310310

311-
/**
312-
* Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion
313-
* is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases
314-
* the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release
315-
* the assertion.
316-
*/
317-
- (void)addRunAssertion;
318-
319-
/**
320-
* Removes an assertion to allow the controller to shutdown once all assertions have been released.
321-
* Invoking this method once all assertions have been released in a noop.
322-
*/
323-
- (void)removeRunAssertion;
324-
325311
/**
326312
* This method returns TRUE if this controller matches the fabric reference and node ID as listed in the parameters.
327313
*/

0 commit comments

Comments
 (0)
Please sign in to comment.