[Darwin] Darwin CI TSAN test failure fix (project-chip#35472)

* [Darwin] MTRDeviceController isSuspended needs to hold _suspensionLock * Changed lock to @synchronized because MTRDevice could call isSuspended while holding the suspension lock * Changed enumeration to be safer * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
DejinChen · Sep 8, 2024 · 9edb3ad · 9edb3ad
1 parent bc19fb0
commit 9edb3ad
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 34 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -134,7 +134,6 @@ @implementation MTRDeviceController {
     MTRP256KeypairBridge _operationalKeypairBridge;
 
     BOOL _suspended;
-    os_unfair_lock _suspensionLock;
 
     // Counters to track assertion status and access controlled by the _assertionLock
     NSUInteger _keepRunningAssertionCounter;
@@ -161,11 +160,11 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
     _shutdownPending = NO;
     _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;
+    // @synchronized(self), so that parts of suspend/resume can't
+    // interleave with each other. Using @synchronized here because
+    // MTRDevice may call isSuspended.
+    _suspended = startSuspended;
 
     _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
 
@@ -212,7 +211,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
         _assertionLock = OS_UNFAIR_LOCK_INIT;
 
         _suspended = startSuspended;
-        _suspensionLock = OS_UNFAIR_LOCK_INIT;
 
         if (storageDelegate != nil) {
             if (storageDelegateQueue == nil) {
@@ -354,50 +352,58 @@ - (BOOL)isRunning
 
 - (BOOL)isSuspended
 {
-    return _suspended;
+    @synchronized(self) {
+        return _suspended;
+    }
 }
 
 - (void)suspend
 {
     MTR_LOG("%@ suspending", self);
 
-    std::lock_guard lock(_suspensionLock);
+    @synchronized(self) {
+        _suspended = YES;
 
-    _suspended = YES;
+        NSMutableArray * devicesToSuspend = [NSMutableArray array];
+        {
+            std::lock_guard lock(*self.deviceMapLock);
+            NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
+            for (MTRDevice * device in devices) {
+                [devicesToSuspend addObject:device];
+            }
+        }
 
-    NSEnumerator * devices;
-    {
-        std::lock_guard lock(*self.deviceMapLock);
-        devices = [self.nodeIDToDeviceMap objectEnumerator];
-    }
+        for (MTRDevice * device in devicesToSuspend) {
+            [device controllerSuspended];
+        }
 
-    for (MTRDevice * device in devices) {
-        [device controllerSuspended];
+        // 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.
     }
-
-    // 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;
+    @synchronized(self) {
+        _suspended = NO;
 
-    NSEnumerator * devices;
-    {
-        std::lock_guard lock(*self.deviceMapLock);
-        devices = [self.nodeIDToDeviceMap objectEnumerator];
-    }
+        NSMutableArray * devicesToResume = [NSMutableArray array];
+        {
+            std::lock_guard lock(*self.deviceMapLock);
+            NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
+            for (MTRDevice * device in devices) {
+                [devicesToResume addObject:device];
+            }
+        }
 
-    for (MTRDevice * device in devices) {
-        [device controllerResumed];
+        for (MTRDevice * device in devicesToResume) {
+            [device controllerResumed];
+        }
     }
 }
 

diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -720,7 +720,7 @@ - (BOOL)_subscriptionsAllowed
     os_unfair_lock_assert_owner(&self->_lock);
 
     // We should not allow a subscription for suspended controllers or device controllers over XPC.
-    return _deviceController.suspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
+    return _deviceController.isSuspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
 }
 
 - (void)_delegateAdded
@@ -1249,7 +1249,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay
 {
     os_unfair_lock_assert_owner(&_lock);
 
-    if (_deviceController.suspended) {
+    if (_deviceController.isSuspended) {
         MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self);
         return;
     }