Darwin: Make MTRSetMessageReliabilityParameters thread-safe (#33527)

* Darwin: Make MTRSetMessageReliabilityParameters thread-safe This can be called from any thread, but the underyling SDK calls need to be made from the Matter queue. Dispatching again in resetOperationalAdvertising was also prone to racing. * Address review comments
project-chip · Aug 8, 2024 · 1237241 · 1237241
1 parent 5c8825c
commit 1237241
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 45 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
@@ -881,24 +881,21 @@ - (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceControll
 
 - (void)resetOperationalAdvertising
 {
-    if (!_advertiseOperational) {
-        // No need to reset anything; we are not advertising the things that
-        // would need to get reset.
-        return;
-    }
+    assertChipStackLockedByCurrentThread();
 
-    std::lock_guard lock(_controllersLock);
-    if (_controllers.count != 0) {
-        // We have a running controller.  That means we likely need to reset
-        // operational advertising for that controller.
-        dispatch_async(_chipWorkQueue, ^{
-            // StartServer() is the only API we have for resetting DNS-SD
-            // advertising.  It sure would be nice if there were a "restart"
-            // that was a no-op if the DNS-SD server was not already
-            // running.
-            app::DnssdServer::Instance().StartServer();
-        });
+    // If we're not advertising, then there's no need to reset anything.
+    VerifyOrReturn(_advertiseOperational);
+
+    // If there are no running controllers there will be no advertisements to reset.
+    {
+        std::lock_guard lock(_controllersLock);
+        VerifyOrReturn(_controllers.count > 0);
     }
+
+    // StartServer() is the only API we have for resetting DNS-SD advertising.
+    // It sure would be nice if there were a "restart" that was a no-op if the
+    // DNS-SD server was not already running.
+    app::DnssdServer::Instance().StartServer();
 }
 
 - (void)controllerShuttingDown:(MTRDeviceController *)controller
@@ -1165,6 +1162,45 @@ - (MTRDeviceController * _Nullable)initializeController:(MTRDeviceController *)c
                                   error:error];
 }
 
+- (void)setMessageReliabilityProtocolIdleRetransmitMs:(nullable NSNumber *)idleRetransmitMs
+                                   activeRetransmitMs:(nullable NSNumber *)activeRetransmitMs
+                                    activeThresholdMs:(nullable NSNumber *)activeThresholdMs
+                          additionalRetransmitDelayMs:(nullable NSNumber *)additionalRetransmitDelayMs
+{
+    [self _assertCurrentQueueIsNotMatterQueue];
+    dispatch_async(_chipWorkQueue, ^{
+        bool resetAdvertising;
+        if (idleRetransmitMs == nil && activeRetransmitMs == nil && activeThresholdMs == nil && additionalRetransmitDelayMs == nil) {
+            Messaging::ReliableMessageMgr::SetAdditionalMRPBackoffTime(NullOptional);
+            resetAdvertising = ReliableMessageProtocolConfig::SetLocalMRPConfig(NullOptional);
+        } else {
+            if (additionalRetransmitDelayMs != nil) {
+                System::Clock::Timeout additionalBackoff(additionalRetransmitDelayMs.unsignedLongValue);
+                Messaging::ReliableMessageMgr::SetAdditionalMRPBackoffTime(MakeOptional(additionalBackoff));
+            }
+
+            // Get current MRP parameters, then override the things we were asked to
+            // override.
+            ReliableMessageProtocolConfig mrpConfig = GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig());
+            if (idleRetransmitMs != nil) {
+                mrpConfig.mIdleRetransTimeout = System::Clock::Milliseconds32(idleRetransmitMs.unsignedLongValue);
+            }
+            if (activeRetransmitMs != nil) {
+                mrpConfig.mActiveRetransTimeout = System::Clock::Milliseconds32(activeRetransmitMs.unsignedLongValue);
+            }
+            if (activeThresholdMs != nil) {
+                mrpConfig.mActiveThresholdTime = System::Clock::Milliseconds32(activeThresholdMs.unsignedLongValue);
+            }
+
+            resetAdvertising = ReliableMessageProtocolConfig::SetLocalMRPConfig(MakeOptional(mrpConfig));
+        }
+
+        if (resetAdvertising) {
+            [self resetOperationalAdvertising];
+        }
+    });
+}
+
 - (PersistentStorageDelegate *)storageDelegate
 {
     return _persistentStorageDelegate;
@@ -1327,33 +1363,8 @@ void MTRSetMessageReliabilityParameters(NSNumber * _Nullable idleRetransmitMs,
     NSNumber * _Nullable activeThresholdMs,
     NSNumber * _Nullable additionalRetransmitDelayMs)
 {
-    bool resetAdvertising = false;
-    if (idleRetransmitMs == nil && activeRetransmitMs == nil && activeThresholdMs == nil && additionalRetransmitDelayMs == nil) {
-        Messaging::ReliableMessageMgr::SetAdditionalMRPBackoffTime(NullOptional);
-        resetAdvertising = ReliableMessageProtocolConfig::SetLocalMRPConfig(NullOptional);
-    } else {
-        if (additionalRetransmitDelayMs != nil) {
-            System::Clock::Timeout additionalBackoff(additionalRetransmitDelayMs.unsignedLongValue);
-            Messaging::ReliableMessageMgr::SetAdditionalMRPBackoffTime(MakeOptional(additionalBackoff));
-        }
-
-        // Get current MRP parameters, then override the things we were asked to
-        // override.
-        ReliableMessageProtocolConfig mrpConfig = GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig());
-        if (idleRetransmitMs != nil) {
-            mrpConfig.mIdleRetransTimeout = System::Clock::Milliseconds32(idleRetransmitMs.unsignedLongValue);
-        }
-        if (activeRetransmitMs != nil) {
-            mrpConfig.mActiveRetransTimeout = System::Clock::Milliseconds32(activeRetransmitMs.unsignedLongValue);
-        }
-        if (activeThresholdMs != nil) {
-            mrpConfig.mActiveThresholdTime = System::Clock::Milliseconds32(activeThresholdMs.unsignedLongValue);
-        }
-
-        resetAdvertising = ReliableMessageProtocolConfig::SetLocalMRPConfig(MakeOptional(mrpConfig));
-    }
-
-    if (resetAdvertising) {
-        [[MTRDeviceControllerFactory sharedInstance] resetOperationalAdvertising];
-    }
+    [MTRDeviceControllerFactory.sharedInstance setMessageReliabilityProtocolIdleRetransmitMs:idleRetransmitMs
+                                                                          activeRetransmitMs:activeThresholdMs
+                                                                           activeThresholdMs:activeThresholdMs
+                                                                 additionalRetransmitDelayMs:additionalRetransmitDelayMs];
 }
diff --git a/src/darwin/Framework/CHIPTests/MTRControllerTests.m b/src/darwin/Framework/CHIPTests/MTRControllerTests.m
@@ -1552,4 +1552,11 @@ - (void)testControllerCATs
     XCTAssertFalse([factory isRunning]);
 }
 
+- (void)testSetMRPParameters
+{
+    // Can be called before starting the factory
+    XCTAssertFalse(MTRDeviceControllerFactory.sharedInstance.running);
+    MTRSetMessageReliabilityParameters(@2000, @2000, @2000, @2000);
+}
+
 @end
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -2035,6 +2035,22 @@ - (void)testControllerServer
     [controllerServer shutdown];
 }
 
+- (void)testSetMRPParametersWithRunningController
+{
+    NSError * error;
+    __auto_type * storageDelegate = [[MTRTestPerControllerStorage alloc] initWithControllerID:[NSUUID UUID]];
+    MTRDeviceController * controller = [self startControllerWithRootKeys:[[MTRTestKeys alloc] init]
+                                                         operationalKeys:[[MTRTestKeys alloc] init]
+                                                                fabricID:@555
+                                                                  nodeID:@888
+                                                                 storage:storageDelegate
+                                                                   error:&error];
+    XCTAssertNotNil(controller);
+    XCTAssertTrue(controller.running);
+    MTRSetMessageReliabilityParameters(@2000, @2000, @2000, @2000);
+    [controller shutdown];
+}
+
 static NSString * const kLocalTestUserDefaultDomain = @"org.csa-iot.matter.darwintest";
 static NSString * const kLocalTestUserDefaultSubscriptionPoolSizeOverrideKey = @"subscriptionPoolSizeOverride";