Skip to content

Commit

Permalink
Remove unreachable shutdown code from MTRDeviceController. (#35689)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple authored Sep 20, 2024
1 parent 9214daa commit d63051a
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 131 deletions.
112 changes: 0 additions & 112 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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()); };
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ - (void)resetOperationalAdvertising
app::DnssdServer::Instance().StartServer();
}

- (void)controllerShuttingDown:(MTRDeviceController *)controller
- (void)controllerShuttingDown:(MTRDeviceController_Concrete *)controller
{
[self _assertCurrentQueueIsNotMatterQueue];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,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
Expand Down
17 changes: 0 additions & 17 deletions src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit d63051a

Please sign in to comment.