Address review comments and add unit test

Author: jtung-apple

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -47,7 +47,6 @@
 #include <app/BufferedReadCallback.h>
 #include <app/ClusterStateCache.h>
 #include <app/InteractionModelEngine.h>
-#include <lib/dnssd/ServiceNaming.h>
 #include <platform/LockTracker.h>
 #include <platform/PlatformManager.h>
 
@@ -672,8 +671,7 @@ - (void)invalidate
     // subscription.  In that case, _internalDeviceState will update when the
     // subscription is actually terminated.
 
-    [_connectivityMonitor stopMonitoring];
-    _connectivityMonitor = nil;
+    [self _stopConnectivityMonitoring];
 
     os_unfair_lock_unlock(&self->_lock);
 }
@@ -868,8 +866,7 @@ - (void)_handleSubscriptionEstablished
     [self _changeState:MTRDeviceStateReachable];
 
     // No need to monitor connectivity after subscription establishment
-    [_connectivityMonitor stopMonitoring];
-    _connectivityMonitor = nil;
+    [self _stopConnectivityMonitoring];
 
     os_unfair_lock_unlock(&self->_lock);
 
@@ -1256,35 +1253,40 @@ - (void)_createDataVersionFilterListFromDictionary:(NSDictionary<MTRClusterPath
 
 - (void)_setupConnectivityMonitoring
 {
-    std::lock_guard lock(_lock);
+    // Dispatch to own queue first to avoid deadlock with syncGetCompressedFabricID
+    dispatch_async(self.queue, ^{
+        // Get the required info before setting up the connectivity monitor
+        NSNumber * compressedFabricID = [self->_deviceController syncGetCompressedFabricID];
+        if (!compressedFabricID) {
+            MTR_LOG_INFO("%@ could not get compressed fabricID", self);
+            return;
+        }
 
-    if (_connectivityMonitor) {
-        // already monitoring
-        return;
-    }
+        // Now lock for _connectivityMonitor
+        std::lock_guard lock(self->_lock);
+        if (self->_connectivityMonitor) {
+            // already monitoring
+            return;
+        }
 
-    // Get the required info before setting up the connectivity monitor
-    NSNumber * compressedFabricID = [_deviceController syncGetCompressedFabricID];
-    if (!compressedFabricID) {
-        MTR_LOG_INFO("%@ could not get compressed fabricID", self);
-        return;
-    }
+        self->_connectivityMonitor = [[MTRDeviceConnectivityMonitor alloc] initWithCompressedFabricID:compressedFabricID nodeID:self.nodeID];
+        [self->_connectivityMonitor startMonitoringWithHandler:^{
+            [self->_deviceController asyncDispatchToMatterQueue:^{
+                [self _triggerResubscribeWithReason:"read-through skipped while not subscribed" nodeLikelyReachable:YES];
+            }
+                                                   errorHandler:nil];
+        } queue:self.queue];
+    });
+}
 
-    char instanceName[chip::Dnssd::kMaxOperationalServiceNameSize];
-    chip::PeerId peerId(static_cast<chip::CompressedFabricId>(compressedFabricID.unsignedLongLongValue), static_cast<chip::NodeId>(_nodeID.unsignedLongLongValue));
-    CHIP_ERROR err = chip::Dnssd::MakeInstanceName(instanceName, sizeof(instanceName), peerId);
-    if (err != CHIP_NO_ERROR) {
-        MTR_LOG_ERROR("%@ could not make instance name", self);
-        return;
-    }
+- (void)_stopConnectivityMonitoring
+{
+    os_unfair_lock_assert_owner(&_lock);
 
-    _connectivityMonitor = [[MTRDeviceConnectivityMonitor alloc] initWithInstanceName:[NSString stringWithUTF8String:instanceName]];
-    [_connectivityMonitor startMonitoringWithHandler:^{
-        [self->_deviceController asyncDispatchToMatterQueue:^{
-            [self _triggerResubscribeWithReason:"read-through skipped while not subscribed" nodeLikelyReachable:YES];
-        }
-                                               errorHandler:nil];
-    } queue:_queue];
+    if (_connectivityMonitor) {
+        [_connectivityMonitor stopMonitoring];
+        _connectivityMonitor = nil;
+    }
 }
 
 // assume lock is held

src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.h

@@ -16,13 +16,27 @@
 
 #import <Foundation/Foundation.h>
 
+#import "MTRDefines_Internal.h"
+
 NS_ASSUME_NONNULL_BEGIN
 
 typedef void (^MTRDeviceConnectivityMonitorHandler)(void);
 
+/**
+ * Class that a matter dns-sd instance name, and monitors connectivity to the device.
+ */
+MTR_TESTABLE
 @interface MTRDeviceConnectivityMonitor : NSObject
-- (instancetype)initWithInstanceName:(NSString *)instanceName;
+- (instancetype)initWithCompressedFabricID:(NSNumber *)compressedFabricID nodeID:(NSNumber *)nodeID;
+
+/**
+ * Any time a path becomes satisfied or route becomes viable, the registered handler will be called.
+ */
 - (void)startMonitoringWithHandler:(MTRDeviceConnectivityMonitorHandler)handler queue:(dispatch_queue_t)queue;
+
+/**
+ * Stops the monitoring. After this method returns no more calls to the handler will be made.
+ */
 - (void)stopMonitoring;
 @end
 

src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm

@@ -22,9 +22,7 @@
 #import <dns_sd.h>
 #import <os/lock.h>
 
-@interface MTRDeviceConnectivityMonitor ()
-- (void)handleResolvedHostname:(const char *)hostName port:(uint16_t)port error:(DNSServiceErrorType)error;
-@end
+#include <lib/dnssd/ServiceNaming.h>
 
 @implementation MTRDeviceConnectivityMonitor {
     NSString * _instanceName;
@@ -38,26 +36,36 @@ @implementation MTRDeviceConnectivityMonitor {
 namespace {
 constexpr char kLocalDot[] = "local.";
 constexpr char kOperationalType[] = "_matter._tcp";
+constexpr int64_t kSharedConnectionLingerIntervalSeconds = (10);
 }
 
-static dispatch_once_t sConnecitivityMonitorOnceToken;
-static os_unfair_lock sConnectivityMonitorLock;
+static os_unfair_lock sConnectivityMonitorLock = OS_UNFAIR_LOCK_INIT;
 static NSUInteger sConnectivityMonitorCount;
 static DNSServiceRef sSharedResolverConnection;
 static dispatch_queue_t sSharedResolverQueue;
 
 - (instancetype)initWithInstanceName:(NSString *)instanceName
 {
     if (self = [super init]) {
-        dispatch_once(&sConnecitivityMonitorOnceToken, ^{
-            sConnectivityMonitorLock = OS_UNFAIR_LOCK_INIT;
-        });
         _instanceName = [instanceName copy];
         _connections = [NSMutableDictionary dictionary];
     }
     return self;
 }
 
+- (instancetype)initWithCompressedFabricID:(NSNumber *)compressedFabricID nodeID:(NSNumber *)nodeID
+{
+    char instanceName[chip::Dnssd::kMaxOperationalServiceNameSize];
+    chip::PeerId peerId(static_cast<chip::CompressedFabricId>(compressedFabricID.unsignedLongLongValue), static_cast<chip::NodeId>(nodeID.unsignedLongLongValue));
+    CHIP_ERROR err = chip::Dnssd::MakeInstanceName(instanceName, sizeof(instanceName), peerId);
+    if (err != CHIP_NO_ERROR) {
+        MTR_LOG_ERROR("%@ could not make instance name", self);
+        return nil;
+    }
+
+    return [self initWithInstanceName:[NSString stringWithUTF8String:instanceName]];
+}
+
 - (void)dealloc
 {
     if (_resolver) {
@@ -76,7 +84,7 @@ + (DNSServiceRef)_sharedResolverConnection
 
     if (!sSharedResolverConnection) {
         DNSServiceErrorType dnsError = DNSServiceCreateConnection(&sSharedResolverConnection);
-        if (dnsError) {
+        if (dnsError != kDNSServiceErr_NoError) {
             MTR_LOG_ERROR("MTRDeviceConnectivityMonitor: DNSServiceCreateConnection failed %d", dnsError);
             return NULL;
         }
@@ -99,7 +107,7 @@ - (void)_callHandler
     os_unfair_lock_assert_owner(&sConnectivityMonitorLock);
     MTRDeviceConnectivityMonitorHandler handlerToCall = self->_monitorHandler;
     if (handlerToCall) {
-        dispatch_async(self->_handlerQueue, ^{ handlerToCall(); });
+        dispatch_async(self->_handlerQueue, handlerToCall);
     }
 }
 
@@ -151,36 +159,37 @@ - (void)handleResolvedHostname:(const char *)hostName port:(uint16_t)port error:
             }
         });
         nw_connection_start(connection);
+
+        _connections[hostNameString] = connection;
     }
 }
 
-static void _resolveReplyCallback(
+static void ResolveCallback(
     DNSServiceRef sdRef,
     DNSServiceFlags flags,
     uint32_t interfaceIndex,
     DNSServiceErrorType errorCode,
-    const char * fullname,
-    const char * hosttarget,
+    const char * fullName,
+    const char * hostName,
     uint16_t port, /* In network byte order */
     uint16_t txtLen,
     const unsigned char * txtRecord,
     void * context)
 {
     auto * connectivityMonitor = (__bridge MTRDeviceConnectivityMonitor *) context;
-    [connectivityMonitor handleResolvedHostname:hosttarget port:port error:errorCode];
+    [connectivityMonitor handleResolvedHostname:hostName port:port error:errorCode];
 }
 
 - (void)startMonitoringWithHandler:(MTRDeviceConnectivityMonitorHandler)handler queue:(dispatch_queue_t)queue
 {
     std::lock_guard lock(sConnectivityMonitorLock);
 
-    MTRDeviceConnectivityMonitorHandler handlerCopy = [handler copy];
-    _monitorHandler = handlerCopy;
+    _monitorHandler = handler;
     _handlerQueue = queue;
 
     // If there's already a resolver running, just return
     if (_resolver) {
-        MTR_LOG_INFO("%@ connectivity monitor updated handler", self);
+        MTR_LOG_INFO("%@ connectivity monitor already running", self);
         return;
     }
 
@@ -197,7 +206,7 @@ - (void)startMonitoringWithHandler:(MTRDeviceConnectivityMonitorHandler)handler
         _instanceName.UTF8String,
         kOperationalType,
         kLocalDot,
-        _resolveReplyCallback,
+        ResolveCallback,
         (__bridge void *) self);
     if (dnsError != kDNSServiceErr_NoError) {
         MTR_LOG_ERROR("%@ failed to create resolver", self);
@@ -207,8 +216,6 @@ - (void)startMonitoringWithHandler:(MTRDeviceConnectivityMonitorHandler)handler
     sConnectivityMonitorCount++;
 }
 
-#define MTRDEVICECONNECTIVITYMONITOR_SHARED_CONNECTION_LINGER_INTERVAL (10)
-
 - (void)_stopMonitoring
 {
     os_unfair_lock_assert_owner(&sConnectivityMonitorLock);
@@ -217,18 +224,21 @@ - (void)_stopMonitoring
     }
     [_connections removeAllObjects];
 
+    _monitorHandler = nil;
+    _handlerQueue = nil;
+
     if (_resolver) {
         DNSServiceRefDeallocate(_resolver);
         _resolver = NULL;
 
         // If no monitor objects exist, schedule to deallocate shared connection and queue
         sConnectivityMonitorCount--;
         if (!sConnectivityMonitorCount) {
-            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (MTRDEVICECONNECTIVITYMONITOR_SHARED_CONNECTION_LINGER_INTERVAL * NSEC_PER_SEC)), sSharedResolverQueue, ^{
+            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kSharedConnectionLingerIntervalSeconds * NSEC_PER_SEC), sSharedResolverQueue, ^{
                 std::lock_guard lock(sConnectivityMonitorLock);
 
                 if (!sConnectivityMonitorCount) {
-                    MTR_LOG_INFO("%@ Closing shared resolver connection", self);
+                    MTR_LOG_INFO("MTRDeviceConnectivityMonitor: Closing shared resolver connection");
                     DNSServiceRefDeallocate(sSharedResolverConnection);
                     sSharedResolverConnection = NULL;
                     sSharedResolverQueue = nil;

src/darwin/Framework/CHIPTests/MTRDeviceConnectivityMonitorTests.m

@@ -0,0 +1,88 @@
+/**
+ *    Copyright (c) 2023 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#import <XCTest/XCTest.h>
+
+#import <Network/Network.h>
+#import <dns_sd.h>
+
+#import "MTRDeviceConnectivityMonitor.h"
+
+@interface MTRDeviceConnectivityMonitor (Test)
+- (instancetype)initWithInstanceName:(NSString *)instanceName;
+@end
+
+@interface MTRDeviceConnectivityMonitorTests : XCTestCase
+@end
+
+@implementation MTRDeviceConnectivityMonitorTests
+
+static DNSServiceRef sSharedConnection;
+
++ (void)setUp
+{
+    DNSServiceErrorType dnsError = DNSServiceCreateConnection(&sSharedConnection);
+    XCTAssertEqual(dnsError, kDNSServiceErr_NoError);
+}
+
++ (void)tearDown
+{
+    DNSServiceRefDeallocate(sSharedConnection);
+}
+
+static char kLocalDot[] = "local.";
+static char kOperationalType[] = "_matter._tcp";
+
+static void test001_MonitorTest_RegisterCallback(
+    DNSServiceRef sdRef,
+    DNSServiceFlags flags,
+    DNSServiceErrorType errorCode,
+    const char * name,
+    const char * regtype,
+    const char * domain,
+    void * context)
+{
+}
+
+- (void)test001_BasicMonitorTest
+{
+    dispatch_queue_t testQueue = dispatch_queue_create("connectivity-monitor-test-queue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
+    DNSServiceRef testAdvertiser;
+    DNSServiceFlags flags = kDNSServiceFlagsNoAutoRename;
+    char testInstanceName[] = "testinstance-name";
+    char testHostName[] = "localhost";
+    uint16_t testPort = htons(15000);
+    DNSServiceErrorType dnsError = DNSServiceRegister(&testAdvertiser, flags, 0, testInstanceName, kOperationalType, kLocalDot, testHostName, testPort, 0, NULL, test001_MonitorTest_RegisterCallback, NULL);
+    XCTAssertEqual(dnsError, kDNSServiceErr_NoError);
+
+    XCTestExpectation * connectivityMonitorCallbackExpectation = [self expectationWithDescription:@"Got connectivity monitor callback"];
+    __block BOOL gotConnectivityMonitorCallback = NO;
+
+    MTRDeviceConnectivityMonitor * monitor = [[MTRDeviceConnectivityMonitor alloc] initWithInstanceName:@(testInstanceName)];
+    [monitor startMonitoringWithHandler:^{
+        if (!gotConnectivityMonitorCallback) {
+            gotConnectivityMonitorCallback = YES;
+            [connectivityMonitorCallbackExpectation fulfill];
+        }
+    } queue:testQueue];
+
+    [self waitForExpectations:@[ connectivityMonitorCallbackExpectation ] timeout:5];
+
+    [monitor stopMonitoring];
+    DNSServiceRefDeallocate(testAdvertiser);
+}
+
+@end