Skip to content

Commit

Permalink
Implement MTRDevice handling in controller suspend/resume.
Browse files Browse the repository at this point in the history
Also blocks acquisition of CASE sessions on a suspended controller, which should
ensure that new requests for MTRDevices and MTRBaseDevices associated with the
controller do not hit the network.
  • Loading branch information
bzbarsky-apple committed Sep 7, 2024
1 parent b3d527e commit 8f443ce
Show file tree
Hide file tree
Showing 10 changed files with 300 additions and 44 deletions.
15 changes: 13 additions & 2 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -648,8 +648,9 @@ - (void)_addDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)que

- (void)_delegateAdded
{
// Nothing to do; this is a hook for subclasses. If that ever changes for
// some reason, subclasses need to start calling this hook on their super.
os_unfair_lock_assert_owner(&self->_lock);

// Nothing to do for now. At the moment this is a hook for subclasses.
}

- (void)removeDelegate:(id<MTRDeviceDelegate>)delegate
Expand Down Expand Up @@ -1743,6 +1744,16 @@ - (NSNumber * _Nullable)_networkFeatures
return result;
}

- (void)controllerSuspended
{
// Nothing to do for now.
}

- (void)controllerResumed
{
// Nothing to do for now.
}

@end

/* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */
Expand Down
48 changes: 39 additions & 9 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ @implementation MTRDeviceController {
MTROperationalCredentialsDelegate * _operationalCredentialsDelegate;
MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
MTRDeviceControllerFactory * _factory;
NSMapTable * _nodeIDToDeviceMap;
os_unfair_lock _underlyingDeviceMapLock;
MTRCommissionableBrowser * _commissionableBrowser;
MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;
Expand All @@ -135,6 +134,7 @@ @implementation MTRDeviceController {
MTRP256KeypairBridge _operationalKeypairBridge;

BOOL _suspended;
os_unfair_lock _suspensionLock;

// Counters to track assertion status and access controlled by the _assertionLock
NSUInteger _keepRunningAssertionCounter;
Expand All @@ -160,6 +160,12 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
_assertionLock = OS_UNFAIR_LOCK_INIT;

_suspended = startSuspended;
// All synchronous suspend/resume activity has to be protected by
// _suspensionLock, so that parts of suspend/resume can't interleave with
// each other.
_suspensionLock = OS_UNFAIR_LOCK_INIT;

_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];

return self;
}
Expand Down Expand Up @@ -204,6 +210,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
_assertionLock = OS_UNFAIR_LOCK_INIT;

_suspended = startSuspended;
_suspensionLock = OS_UNFAIR_LOCK_INIT;

if (storageDelegate != nil) {
if (storageDelegateQueue == nil) {
Expand Down Expand Up @@ -350,23 +357,46 @@ - (BOOL)isSuspended

- (void)suspend
{
MTR_LOG("%@ suspending", self);

std::lock_guard lock(_suspensionLock);

_suspended = YES;

// TODO: In the concrete class (which is unused so far!), iterate our
// MTRDevices, tell them to tear down subscriptions. Possibly close all
// CASE sessions for our identity. Possibly try to see whether we can
// change our fabric entry to not advertise and restart advertising.
NSEnumerator * devices;
{
std::lock_guard lock(*self.deviceMapLock);
devices = [self.nodeIDToDeviceMap objectEnumerator];
}

for (MTRDevice * device in devices) {
[device controllerSuspended];
}

// TODO: What should happen with active commissioning sessions? Presumably
// close them?
// TODO: In the concrete class, consider what should happen with:
//
// * Active commissioning sessions (presumably close them?)
// * CASE sessions in general.
// * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
}

- (void)resume
{
MTR_LOG("%@ resuming", self);

std::lock_guard lock(_suspensionLock);

_suspended = NO;

// TODO: In the concrete class (which is unused so far!), iterate our
// MTRDevices, tell them to restart subscriptions.
NSEnumerator * devices;
{
std::lock_guard lock(*self.deviceMapLock);
devices = [self.nodeIDToDeviceMap objectEnumerator];
}

for (MTRDevice * device in devices) {
[device controllerResumed];
}
}

- (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate
Expand Down
5 changes: 5 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,11 @@ MTR_AVAILABLE(ios(17.6), macos(14.6), watchos(10.6), tvos(17.6))
intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate
rootCertificate:(MTRCertificateDERBytes)rootCertificate;

/**
* The root certificate we were initialized with.
*/
@property (nonatomic, copy, readonly) MTRCertificateDERBytes rootCertificate MTR_NEWLY_AVAILABLE;

@end

MTR_NEWLY_AVAILABLE
Expand Down
3 changes: 3 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,9 @@ + (nullable NSData *)publicKeyFromCertificate:(MTRCertificateDERBytes)certificat
@end

@implementation MTRDeviceControllerExternalCertificateParameters

@dynamic rootCertificate;

- (instancetype)initWithStorageDelegate:(id<MTRDeviceControllerStorageDelegate>)storageDelegate
storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue
uniqueIdentifier:(NSUUID *)uniqueIdentifier
Expand Down
20 changes: 18 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
_otaProviderDelegateQueue = otaProviderDelegateQueue;
_chipWorkQueue = queue;
_factory = factory;
// TODO: Shouldn't nodeIDToDeviceMap just be set up by initForSubclasses?
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
_serverEndpoints = [[NSMutableArray alloc] init];
_commissionableBrowser = nil;

Expand Down Expand Up @@ -1416,6 +1414,15 @@ - (BOOL)checkIsRunning:(NSError * __autoreleasing *)error

- (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
{
// TODO: Figure out whether the synchronization here makes sense. What
// happens if this call happens mid-suspend or mid-resume?
if (self.suspended) {
MTR_LOG_ERROR("%@ suspended: can't get session for node %016llX-%016llx (%llu)", self, self.compressedFabricID.unsignedLongLongValue, nodeID, nodeID);
// TODO: Can we do a better error here?
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil);
return;
}

// Get the corresponding MTRDevice object to determine if the case/subscription pool is to be used
MTRDevice * device = [self deviceForNodeID:@(nodeID)];

Expand All @@ -1440,6 +1447,15 @@ - (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConn

- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
{
// TODO: Figure out whether the synchronization here makes sense. What
// happens if this call happens mid-suspend or mid-resume?
if (self.suspended) {
MTR_LOG_ERROR("%@ suspended: can't get session for node %016llX-%016llx (%llu)", self, self.compressedFabricID.unsignedLongLongValue, nodeID, nodeID);
// TODO: Can we do a better error here?
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil);
return;
}

[self
asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) {
auto connectionBridge = new MTRDeviceConnectionBridge(completion);
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ NS_ASSUME_NONNULL_BEGIN

@interface MTRDeviceController ()

@property (nonatomic, readwrite, nullable) NSMapTable * nodeIDToDeviceMap;
@property (nonatomic, readonly) NSMapTable<NSNumber *, MTRDevice *> * nodeIDToDeviceMap;
@property (readonly, assign) os_unfair_lock_t deviceMapLock;

// queue used to serialize all work performed by the MTRDeviceController
Expand Down
1 change: 0 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete
self.xpcConnection = connectionBlock();
self.uniqueIdentifier = UUID;
self.chipWorkQueue = dispatch_queue_create("MTRDeviceController_XPC_queue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];

MTR_LOG("Set up XPC Connection: %@", self.xpcConnection);
if (self.xpcConnection) {
Expand Down
50 changes: 46 additions & 4 deletions src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
Original file line number Diff line number Diff line change
Expand Up @@ -724,14 +724,23 @@ - (BOOL)_subscriptionsAllowed
{
os_unfair_lock_assert_owner(&self->_lock);

// We should not allow a subscription for device controllers over XPC.
return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
// We should not allow a subscription for suspended controllers or device controllers over XPC.
return _deviceController.suspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
}

- (void)_delegateAdded
{
os_unfair_lock_assert_owner(&self->_lock);

[super _delegateAdded];

[self _ensureSubscriptionForExistingDelegates:@"delegate is set"];
}

- (void)_ensureSubscriptionForExistingDelegates:(NSString *)reason
{
os_unfair_lock_assert_owner(&self->_lock);

__block BOOL shouldSetUpSubscription = [self _subscriptionsAllowed];

// For unit testing only. If this ever changes to not being for unit testing purposes,
Expand All @@ -754,10 +763,10 @@ - (void)_delegateAdded
MTR_LOG(" => %@ - device is a thread device, scheduling in pool", self);
[self _scheduleSubscriptionPoolWork:^{
std::lock_guard lock(self->_lock);
[self _setupSubscriptionWithReason:@"delegate is set and scheduled subscription is happening"];
[self _setupSubscriptionWithReason:[NSString stringWithFormat:@"%@ and scheduled subscription is happening", reason]];
} inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"];
} else {
[self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"];
[self _setupSubscriptionWithReason:[NSString stringWithFormat:@"%@ and subscription is needed", reason]];
}
}
}
Expand Down Expand Up @@ -1243,6 +1252,11 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay
{
os_unfair_lock_assert_owner(&_lock);

if (_deviceController.suspended) {
MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self);
return;
}

// If we are here, then either we failed to establish initial CASE, or we
// failed to send the initial SubscribeRequest message, or our ReadClient
// has given up completely. Those all count as "we have tried and failed to
Expand Down Expand Up @@ -4002,6 +4016,34 @@ - (NSNumber * _Nullable)_networkFeatures
return result;
}

- (void)controllerSuspended
{
[super controllerSuspended];

std::lock_guard lock(self->_lock);
[self _resetSubscriptionWithReasonString:@"Controller suspended"];

// Ensure that any pre-existing resubscribe attempts we control don't try to
// do anything.
_reattemptingSubscription = NO;
}

- (void)controllerResumed
{
[super controllerResumed];

std::lock_guard lock(self->_lock);

if (![self _delegateExists]) {
MTR_LOG("%@ ignoring controller resume: no delegates", self);
return;
}

// Use _ensureSubscriptionForExistingDelegates so that the subscriptions
// will go through the pool as needed, not necessarily happen immediately.
[self _ensureSubscriptionForExistingDelegates:@"Controller resumed"];
}

@end

/* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */
Expand Down
7 changes: 7 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ MTR_DIRECT_MEMBERS

- (BOOL)_delegateExists;

// Must be called by subclasses or MTRDevice implementation only.
- (void)_delegateAdded;

#ifdef DEBUG
// Only used for unit test purposes - normal delegate should not expect or handle being called back synchronously
// Returns YES if a delegate is called
Expand All @@ -205,6 +208,10 @@ MTR_DIRECT_MEMBERS
// Used to generate attribute report that contains all known attributes, taking into consideration expected values
- (NSArray<NSDictionary<NSString *, id> *> *)getAllAttributesReport;

// Hooks for controller suspend/resume.
- (void)controllerSuspended;
- (void)controllerResumed;

@end

#pragma mark - MTRDevice internal state monitoring
Expand Down
Loading

0 comments on commit 8f443ce

Please sign in to comment.