Skip to content

Commit 3db748f

Browse files
Implement MTRDevice handling in controller suspend/resume.
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.
1 parent 18466d0 commit 3db748f

10 files changed

+300
-39
lines changed

src/darwin/Framework/CHIP/MTRDevice.mm

+13-2
Original file line numberDiff line numberDiff line change
@@ -648,8 +648,9 @@ - (void)_addDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)que
648648

649649
- (void)_delegateAdded
650650
{
651-
// Nothing to do; this is a hook for subclasses. If that ever changes for
652-
// some reason, subclasses need to start calling this hook on their super.
651+
os_unfair_lock_assert_owner(&self->_lock);
652+
653+
// Nothing to do for now. At the moment this is a hook for subclasses.
653654
}
654655

655656
- (void)removeDelegate:(id<MTRDeviceDelegate>)delegate
@@ -1732,6 +1733,16 @@ - (NSNumber * _Nullable)_networkFeatures
17321733
return result;
17331734
}
17341735

1736+
- (void)controllerSuspended
1737+
{
1738+
// Nothing to do for now.
1739+
}
1740+
1741+
- (void)controllerResumed
1742+
{
1743+
// Nothing to do for now.
1744+
}
1745+
17351746
@end
17361747

17371748
/* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */

src/darwin/Framework/CHIP/MTRDeviceController.mm

+39-4
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,6 @@ @implementation MTRDeviceController {
120120
MTROperationalCredentialsDelegate * _operationalCredentialsDelegate;
121121
MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
122122
MTRDeviceControllerFactory * _factory;
123-
NSMapTable * _nodeIDToDeviceMap;
124123
os_unfair_lock _underlyingDeviceMapLock;
125124
MTRCommissionableBrowser * _commissionableBrowser;
126125
MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;
@@ -135,6 +134,7 @@ @implementation MTRDeviceController {
135134
MTRP256KeypairBridge _operationalKeypairBridge;
136135

137136
BOOL _suspended;
137+
os_unfair_lock _suspensionLock;
138138

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

162162
_suspended = startSuspended;
163+
// All synchronous suspend/resume activity has to be protected by
164+
// _suspensionLock, so that parts of suspend/resume can't interleave with
165+
// each other.
166+
_suspensionLock = OS_UNFAIR_LOCK_INIT;
167+
168+
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
163169

164170
return self;
165171
}
@@ -204,6 +210,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
204210
_assertionLock = OS_UNFAIR_LOCK_INIT;
205211

206212
_suspended = startSuspended;
213+
_suspensionLock = OS_UNFAIR_LOCK_INIT;
207214

208215
if (storageDelegate != nil) {
209216
if (storageDelegateQueue == nil) {
@@ -350,9 +357,26 @@ - (BOOL)isSuspended
350357

351358
- (void)suspend
352359
{
360+
MTR_LOG("%@ suspending", self);
361+
362+
std::lock_guard lock(_suspensionLock);
363+
353364
_suspended = YES;
354365

355-
// TODO: In the concrete class (which is unused so far!), iterate our
366+
NSEnumerator * devices;
367+
{
368+
std::lock_guard lock(*self.deviceMapLock);
369+
devices = [self.nodeIDToDeviceMap objectEnumerator];
370+
}
371+
372+
for (MTRDevice * device in devices) {
373+
[device controllerSuspended];
374+
}
375+
376+
// TODO: In the concrete class, consider what should happen with:
377+
//
378+
// * Active commissioning sessions (presumably close them?)
379+
// * CASE sessions in general.
356380
// MTRDevices, tell them to tear down subscriptions. Possibly close all
357381
// CASE sessions for our identity. Possibly try to see whether we can
358382
// change our fabric entry to not advertise and restart advertising.
@@ -363,10 +387,21 @@ - (void)suspend
363387

364388
- (void)resume
365389
{
390+
MTR_LOG("%@ resuming", self);
391+
392+
std::lock_guard lock(_suspensionLock);
393+
366394
_suspended = NO;
367395

368-
// TODO: In the concrete class (which is unused so far!), iterate our
369-
// MTRDevices, tell them to restart subscriptions.
396+
NSEnumerator * devices;
397+
{
398+
std::lock_guard lock(*self.deviceMapLock);
399+
devices = [self.nodeIDToDeviceMap objectEnumerator];
400+
}
401+
402+
for (MTRDevice * device in devices) {
403+
[device controllerResumed];
404+
}
370405
}
371406

372407
- (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate

src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h

+5
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ MTR_AVAILABLE(ios(17.6), macos(14.6), watchos(10.6), tvos(17.6))
150150
intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate
151151
rootCertificate:(MTRCertificateDERBytes)rootCertificate;
152152

153+
/**
154+
* The root certificate we were initialized with.
155+
*/
156+
@property (nonatomic, copy, readonly) MTRCertificateDERBytes rootCertificate MTR_NEWLY_AVAILABLE;
157+
153158
@end
154159

155160
MTR_NEWLY_AVAILABLE

src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm

+3
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,9 @@ + (nullable NSData *)publicKeyFromCertificate:(MTRCertificateDERBytes)certificat
338338
@end
339339

340340
@implementation MTRDeviceControllerExternalCertificateParameters
341+
342+
@dynamic rootCertificate;
343+
341344
- (instancetype)initWithStorageDelegate:(id<MTRDeviceControllerStorageDelegate>)storageDelegate
342345
storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue
343346
uniqueIdentifier:(NSUUID *)uniqueIdentifier

src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm

+18-2
Original file line numberDiff line numberDiff line change
@@ -288,8 +288,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
288288
_otaProviderDelegateQueue = otaProviderDelegateQueue;
289289
_chipWorkQueue = queue;
290290
_factory = factory;
291-
// TODO: Shouldn't nodeIDToDeviceMap just be set up by initForSubclasses?
292-
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
293291
_serverEndpoints = [[NSMutableArray alloc] init];
294292
_commissionableBrowser = nil;
295293

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

14171415
- (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
14181416
{
1417+
// TODO: Figure out whether the synchronization here makes sense. What
1418+
// happens if this call happens mid-suspend or mid-resume?
1419+
if (self.suspended) {
1420+
MTR_LOG_ERROR("%@ suspended: can't get session for node %016llX-%016llx (%llu)", self, self.compressedFabricID.unsignedLongLongValue, nodeID, nodeID);
1421+
// TODO: Can we do a better error here?
1422+
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil);
1423+
return;
1424+
}
1425+
14191426
// Get the corresponding MTRDevice object to determine if the case/subscription pool is to be used
14201427
MTRDevice * device = [self deviceForNodeID:@(nodeID)];
14211428

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

14411448
- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
14421449
{
1450+
// TODO: Figure out whether the synchronization here makes sense. What
1451+
// happens if this call happens mid-suspend or mid-resume?
1452+
if (self.suspended) {
1453+
MTR_LOG_ERROR("%@ suspended: can't get session for node %016llX-%016llx (%llu)", self, self.compressedFabricID.unsignedLongLongValue, nodeID, nodeID);
1454+
// TODO: Can we do a better error here?
1455+
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil);
1456+
return;
1457+
}
1458+
14431459
[self
14441460
asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) {
14451461
auto connectionBridge = new MTRDeviceConnectionBridge(completion);

src/darwin/Framework/CHIP/MTRDeviceController_Internal.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ NS_ASSUME_NONNULL_BEGIN
6666

6767
@interface MTRDeviceController ()
6868

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

7272
// queue used to serialize all work performed by the MTRDeviceController

src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm

-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete
113113
self.xpcConnection = connectionBlock();
114114
self.uniqueIdentifier = UUID;
115115
self.chipWorkQueue = dispatch_queue_create("MTRDeviceController_XPC_queue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
116-
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
117116

118117
MTR_LOG("Set up XPC Connection: %@", self.xpcConnection);
119118
if (self.xpcConnection) {

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

+46-4
Original file line numberDiff line numberDiff line change
@@ -724,14 +724,23 @@ - (BOOL)_subscriptionsAllowed
724724
{
725725
os_unfair_lock_assert_owner(&self->_lock);
726726

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

731731
- (void)_delegateAdded
732732
{
733733
os_unfair_lock_assert_owner(&self->_lock);
734734

735+
[super _delegateAdded];
736+
737+
[self _ensureSubscriptionForExistingDelegates:@"delegate is set"];
738+
}
739+
740+
- (void)_ensureSubscriptionForExistingDelegates:(NSString *)reason
741+
{
742+
os_unfair_lock_assert_owner(&self->_lock);
743+
735744
__block BOOL shouldSetUpSubscription = [self _subscriptionsAllowed];
736745

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

1255+
if (_deviceController.suspended) {
1256+
MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self);
1257+
return;
1258+
}
1259+
12461260
// If we are here, then either we failed to establish initial CASE, or we
12471261
// failed to send the initial SubscribeRequest message, or our ReadClient
12481262
// has given up completely. Those all count as "we have tried and failed to
@@ -3975,6 +3989,34 @@ - (NSNumber * _Nullable)_networkFeatures
39753989
return result;
39763990
}
39773991

3992+
- (void)controllerSuspended
3993+
{
3994+
[super controllerSuspended];
3995+
3996+
std::lock_guard lock(self->_lock);
3997+
[self _resetSubscriptionWithReasonString:@"Controller suspended"];
3998+
3999+
// Ensure that any pre-existing resubscribe attempts we control don't try to
4000+
// do anything.
4001+
_reattemptingSubscription = NO;
4002+
}
4003+
4004+
- (void)controllerResumed
4005+
{
4006+
[super controllerResumed];
4007+
4008+
std::lock_guard lock(self->_lock);
4009+
4010+
if (![self _delegateExists]) {
4011+
MTR_LOG("%@ ignoring controller resume: no delegates", self);
4012+
return;
4013+
}
4014+
4015+
// Use _ensureSubscriptionForExistingDelegates so that the subscriptions
4016+
// will go through the pool as needed, not necessarily happen immediately.
4017+
[self _ensureSubscriptionForExistingDelegates:@"Controller resumed"];
4018+
}
4019+
39784020
@end
39794021

39804022
/* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */

src/darwin/Framework/CHIP/MTRDevice_Internal.h

+7
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,19 @@ MTR_DIRECT_MEMBERS
196196

197197
- (BOOL)_delegateExists;
198198

199+
// Must be called by subclasses or MTRDevice implementation only.
200+
- (void)_delegateAdded;
201+
199202
#ifdef DEBUG
200203
// Only used for unit test purposes - normal delegate should not expect or handle being called back synchronously
201204
// Returns YES if a delegate is called
202205
- (void)_callFirstDelegateSynchronouslyWithBlock:(void (^)(id<MTRDeviceDelegate> delegate))block;
203206
#endif
204207

208+
// Hooks for controller suspend/resume.
209+
- (void)controllerSuspended;
210+
- (void)controllerResumed;
211+
205212
@end
206213

207214
#pragma mark - MTRDevice internal state monitoring

0 commit comments

Comments
 (0)