fix: improve Reachability test and referencing (#3342)

Co-authored-by: Philipp Hofmann <[email protected]>
getsentry · Oct 19, 2023 · 209e288 · 209e288
1 parent 62c15d4
commit 209e288
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 73 deletions.
diff --git a/Sources/Sentry/SentryReachability.m b/Sources/Sentry/SentryReachability.m
@@ -124,13 +124,6 @@
     }
 }
 
-void
-SentryConnectivityReset(void)
-{
-    [sentry_reachability_observers removeAllObjects];
-    sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
-}
-
 @implementation SentryReachability
 
 + (void)initialize
@@ -172,6 +165,10 @@ - (void)addObserver:(id<SentryReachabilityObserver>)observer;
 
         sentry_reachability_queue
             = dispatch_queue_create("io.sentry.cocoa.connectivity", DISPATCH_QUEUE_SERIAL);
+        // Ensure to call CFRelease for the return value of SCNetworkReachabilityCreateWithName, see
+        // https://developer.apple.com/documentation/systemconfiguration/1514904-scnetworkreachabilitycreatewithn?language=objc
+        // and
+        // https://developer.apple.com/documentation/systemconfiguration/scnetworkreachability?language=objc
         _sentry_reachability_ref = SCNetworkReachabilityCreateWithName(NULL, "sentry.io");
         if (!_sentry_reachability_ref) { // Can be null if a bad hostname was specified
             return;
@@ -191,7 +188,9 @@ - (void)removeObserver:(id<SentryReachabilityObserver>)observer
         SENTRY_LOG_DEBUG(@"Synchronized to remove observer: %@", observer);
         [sentry_reachability_observers removeObject:observer];
 
-        [self unsetReachabilityCallbackIfNeeded];
+        if (sentry_reachability_observers.count == 0) {
+            [self unsetReachabilityCallback];
+        }
     }
 }
 
@@ -201,29 +200,19 @@ - (void)removeAllObservers
     @synchronized(sentry_reachability_observers) {
         SENTRY_LOG_DEBUG(@"Synchronized to remove all observers.");
         [sentry_reachability_observers removeAllObjects];
-        [self unsetReachabilityCallbackIfNeeded];
+        [self unsetReachabilityCallback];
     }
 }
 
-- (void)unsetReachabilityCallbackIfNeeded
+- (void)unsetReachabilityCallback
 {
-    if (sentry_reachability_observers.count > 0) {
-        SENTRY_LOG_DEBUG(
-            @"Other observers still registered, will not unset reachability callback.");
-        return;
-    }
-
-    if (!self.setReachabilityCallback) {
-        SENTRY_LOG_DEBUG(@"Skipping unsetting reachability callback.");
-        return;
-    }
-
     sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
 
     if (_sentry_reachability_ref != nil) {
         SENTRY_LOG_DEBUG(@"removing callback for reachability ref %@", _sentry_reachability_ref);
         SCNetworkReachabilitySetCallback(_sentry_reachability_ref, NULL, NULL);
         SCNetworkReachabilitySetDispatchQueue(_sentry_reachability_ref, NULL);
+        CFRelease(_sentry_reachability_ref);
         _sentry_reachability_ref = nil;
     }
 

diff --git a/Tests/SentryTests/Networking/SentryReachabilityTests.m b/Tests/SentryTests/Networking/SentryReachabilityTests.m
@@ -3,27 +3,25 @@
 #import "SentryReachability.h"
 #import <XCTest/XCTest.h>
 
-void SentryConnectivityReset(void);
-
 @interface TestSentryReachabilityObserver : NSObject <SentryReachabilityObserver>
 
-@property (strong, nonatomic) XCTestExpectation *expectation;
+@property (assign, nonatomic) NSUInteger connectivityChangedInvocations;
 
 @end
 @implementation TestSentryReachabilityObserver
 
-- (instancetype)initWithExpectation:(XCTestExpectation *)expectation
+- (instancetype)init
 {
     if (self = [super init]) {
-        self.expectation = expectation;
+        self.connectivityChangedInvocations = 0;
     }
     return self;
 }
 
 - (void)connectivityChanged:(BOOL)connected typeDescription:(nonnull NSString *)typeDescription
 {
     NSLog(@"Received connectivity notification: %i; type: %@", connected, typeDescription);
-    [self.expectation fulfill];
+    self.connectivityChangedInvocations++;
 }
 
 @end
@@ -45,8 +43,8 @@ - (void)setUp
 
 - (void)tearDown
 {
+    [self.reachability removeAllObservers];
     self.reachability = nil;
-    SentryConnectivityReset();
 }
 
 - (void)testConnectivityRepresentations
@@ -71,89 +69,76 @@ - (void)testConnectivityRepresentations
 
 - (void)testMultipleReachabilityObservers
 {
-    XCTestExpectation *aExp =
-        [self expectationWithDescription:
-                  @"reachability state change for observer monitoring https://sentry.io"];
-    aExp.expectedFulfillmentCount = 5;
-    TestSentryReachabilityObserver *a =
-        [[TestSentryReachabilityObserver alloc] initWithExpectation:aExp];
-    [self.reachability addObserver:a];
+    NSLog(@"[Sentry] [TEST] creating observer A");
+    TestSentryReachabilityObserver *observerA = [[TestSentryReachabilityObserver alloc] init];
+    NSLog(@"[Sentry] [TEST] adding observer A as reachability observer");
+    [self.reachability addObserver:observerA];
 
+    NSLog(@"[Sentry] [TEST] throwaway reachability callback, setting to reachable");
     SentryConnectivityCallback(self.reachability.sentry_reachability_ref,
         kSCNetworkReachabilityFlagsReachable, nil); // ignored, as it's the first callback
+    NSLog(@"[Sentry] [TEST] reachability callback to set to intervention required");
     SentryConnectivityCallback(self.reachability.sentry_reachability_ref,
         kSCNetworkReachabilityFlagsInterventionRequired, nil);
 
-    XCTestExpectation *bExp =
-        [self expectationWithDescription:
-                  @"reachability state change for observer monitoring https://google.io"];
-    bExp.expectedFulfillmentCount = 2;
-    TestSentryReachabilityObserver *b =
-        [[TestSentryReachabilityObserver alloc] initWithExpectation:bExp];
-    [self.reachability addObserver:b];
+    NSLog(@"[Sentry] [TEST] creating observer B");
+    TestSentryReachabilityObserver *observerB = [[TestSentryReachabilityObserver alloc] init];
+    NSLog(@"[Sentry] [TEST] adding observer B as reachability observer");
+    [self.reachability addObserver:observerB];
 
+    NSLog(@"[Sentry] [TEST] reachability callback to set to back to reachable");
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
+    NSLog(@"[Sentry] [TEST] reachability callback to set to back to intervention required");
     SentryConnectivityCallback(self.reachability.sentry_reachability_ref,
         kSCNetworkReachabilityFlagsInterventionRequired, nil);
 
-    [self.reachability removeObserver:b];
+    NSLog(@"[Sentry] [TEST] removing observer B as reachability observer");
+    [self.reachability removeObserver:observerB];
 
+    NSLog(@"[Sentry] [TEST] reachability callback to set to back to reachable");
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
 
-    [self waitForExpectations:@[ aExp, bExp ] timeout:1.0];
+    XCTAssertEqual(5, observerA.connectivityChangedInvocations);
+    XCTAssertEqual(2, observerB.connectivityChangedInvocations);
 
-    [self.reachability removeObserver:a];
+    NSLog(@"[Sentry] [TEST] removing observer A as reachability observer");
+    [self.reachability removeObserver:observerA];
 }
 
 - (void)testNoObservers
 {
-    XCTestExpectation *aExp =
-        [self expectationWithDescription:
-                  @"reachability state change for observer monitoring https://sentry.io"];
-    [aExp setInverted:YES];
-    TestSentryReachabilityObserver *a =
-        [[TestSentryReachabilityObserver alloc] initWithExpectation:aExp];
-    [self.reachability addObserver:a];
-    [self.reachability removeObserver:a];
+    TestSentryReachabilityObserver *observer = [[TestSentryReachabilityObserver alloc] init];
+    [self.reachability addObserver:observer];
+    [self.reachability removeObserver:observer];
 
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
 
-    [self waitForExpectations:@[ aExp ] timeout:1.0];
+    XCTAssertEqual(0, observer.connectivityChangedInvocations);
 
     [self.reachability removeAllObservers];
 }
 
 - (void)testReportSameObserver_OnlyCalledOnce
 {
-    XCTestExpectation *aExp =
-        [self expectationWithDescription:
-                  @"reachability state change for observer monitoring https://sentry.io"];
-    aExp.expectedFulfillmentCount = 1;
-    TestSentryReachabilityObserver *a =
-        [[TestSentryReachabilityObserver alloc] initWithExpectation:aExp];
-    [self.reachability addObserver:a];
-    [self.reachability addObserver:a];
+    TestSentryReachabilityObserver *observer = [[TestSentryReachabilityObserver alloc] init];
+    [self.reachability addObserver:observer];
+    [self.reachability addObserver:observer];
 
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
 
-    [self waitForExpectations:@[ aExp ] timeout:1.0];
+    XCTAssertEqual(1, observer.connectivityChangedInvocations);
 
-    [self.reachability removeObserver:a];
+    [self.reachability removeObserver:observer];
 }
 
 - (void)testReportSameReachabilityState_OnlyCalledOnce
 {
-    XCTestExpectation *aExp =
-        [self expectationWithDescription:
-                  @"reachability state change for observer monitoring https://sentry.io"];
-    aExp.expectedFulfillmentCount = 1;
-    TestSentryReachabilityObserver *a =
-        [[TestSentryReachabilityObserver alloc] initWithExpectation:aExp];
-    [self.reachability addObserver:a];
+    TestSentryReachabilityObserver *observer = [[TestSentryReachabilityObserver alloc] init];
+    [self.reachability addObserver:observer];
 
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
@@ -162,9 +147,9 @@ - (void)testReportSameReachabilityState_OnlyCalledOnce
     SentryConnectivityCallback(
         self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil);
 
-    [self waitForExpectations:@[ aExp ] timeout:1.0];
+    XCTAssertEqual(1, observer.connectivityChangedInvocations);
 
-    [self.reachability removeObserver:a];
+    [self.reachability removeObserver:observer];
 }
 
 @end