fix: Multiple issues in SentryReachability (#3338)

getsentry · Oct 11, 2023 · 1437c68 · 1437c68
1 parent 2401cbd
commit 1437c68
Show file tree

Hide file tree

Showing 14 changed files with 305 additions and 102 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - Always start SDK on the main thread (#3291)
 - App hang with race condition for tick counter (#3290)
 - Remove "duplicate library" warning (#3312)
+- Fix multiple issues in Reachability (#3338)
 - Remove unnecessary build settings (#3325)
 
 ## 8.13.0

diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj
@@ -84,6 +84,7 @@
 		627E7589299F6FE40085504D /* SentryInternalDefines.h in Headers */ = {isa = PBXBuildFile; fileRef = 627E7588299F6FE40085504D /* SentryInternalDefines.h */; };
 		62885DA729E946B100554F38 /* TestConncurrentModifications.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62885DA629E946B100554F38 /* TestConncurrentModifications.swift */; };
 		62950F1029E7FE0100A42624 /* SentryTransactionContextTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62950F0F29E7FE0100A42624 /* SentryTransactionContextTests.swift */; };
+		629690532AD3E060000185FA /* SentryReachabilitySwiftTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 629690522AD3E060000185FA /* SentryReachabilitySwiftTests.swift */; };
 		62B86CFC29F052BB008F3947 /* SentryTestLogConfig.m in Sources */ = {isa = PBXBuildFile; fileRef = 62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */; };
 		62E081A929ED4260000F69FC /* SentryBreadcrumbDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */; };
 		62E081AB29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */; };
@@ -964,6 +965,7 @@
 		627E7588299F6FE40085504D /* SentryInternalDefines.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryInternalDefines.h; path = include/SentryInternalDefines.h; sourceTree = "<group>"; };
 		62885DA629E946B100554F38 /* TestConncurrentModifications.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestConncurrentModifications.swift; sourceTree = "<group>"; };
 		62950F0F29E7FE0100A42624 /* SentryTransactionContextTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryTransactionContextTests.swift; sourceTree = "<group>"; };
+		629690522AD3E060000185FA /* SentryReachabilitySwiftTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryReachabilitySwiftTests.swift; sourceTree = "<group>"; };
 		62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryTestLogConfig.m; sourceTree = "<group>"; };
 		62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBreadcrumbDelegate.h; path = include/SentryBreadcrumbDelegate.h; sourceTree = "<group>"; };
 		62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBreadcrumbTestDelegate.swift; sourceTree = "<group>"; };
@@ -2742,6 +2744,7 @@
 				7B5CAF7C27F5AD0600ED0DB6 /* TestNSURLRequestBuilder.h */,
 				7B5CAF7D27F5AD3500ED0DB6 /* TestNSURLRequestBuilder.m */,
 				0AE455AC28F584D2006680E5 /* SentryReachabilityTests.m */,
+				629690522AD3E060000185FA /* SentryReachabilitySwiftTests.swift */,
 				0A9415B928F96CAC006A5DD1 /* TestSentryReachability.swift */,
 			);
 			path = Networking;
@@ -4209,6 +4212,7 @@
 				8ED3D306264DFE700049393B /* SwiftDescriptorTests.swift in Sources */,
 				7BFC16AD2524BCE700FF6266 /* SentryMessageTests.swift in Sources */,
 				7BF6505F292B77EC00BBA5A8 /* SentryMetricKitIntegrationTests.swift in Sources */,
+				629690532AD3E060000185FA /* SentryReachabilitySwiftTests.swift in Sources */,
 				7BA61CC6247CFC5F00C130A8 /* SentryCrashDefaultBinaryImageProviderTests.swift in Sources */,
 				7BBC827925DFD7D7005F1ED8 /* SentryInAppLogicTests.swift in Sources */,
 				7BD7299A2463EA4A00EA3610 /* SentryDateUtilTests.swift in Sources */,

diff --git a/SentryTestUtils/ClearTestState.swift b/SentryTestUtils/ClearTestState.swift
@@ -30,8 +30,13 @@ class TestCleanup: NSObject {
         SentryAppStartTracker.load()
         SentryUIViewControllerPerformanceTracker.shared.enableWaitForFullDisplay = false
         SentryDependencyContainer.sharedInstance().swizzleWrapper.removeAllCallbacks()
+
         #endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
-
+
+#if !os(watchOS)
+        SentryDependencyContainer.sharedInstance().reachability.removeAllObservers()
+#endif // !os(watchOS)
+
         SentryDependencyContainer.reset()
         Dynamic(SentryGlobalEventProcessor.shared()).removeAllProcessors()
         SentryPerformanceTracker.shared.clear()

diff --git a/SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h b/SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h
@@ -33,6 +33,7 @@
 #import "SentryNetworkTracker.h"
 #import "SentryPerformanceTracker+Testing.h"
 #import "SentryRandom.h"
+#import "SentryReachability.h"
 #import "SentrySDK+Private.h"
 #import "SentrySDK+Tests.h"
 #import "SentrySession.h"

diff --git a/Sources/Sentry/SentryBreadcrumbTracker.m b/Sources/Sentry/SentryBreadcrumbTracker.m
@@ -67,6 +67,9 @@ - (void)stop
         removeSwizzleSendActionForKey:SentryBreadcrumbTrackerSwizzleSendAction];
 #endif // SENTRY_HAS_UIKIT
     _delegate = nil;
+#if !TARGET_OS_WATCH
+    [self stopTrackNetworkConnectivityChanges];
+#endif // !TARGET_OS_WATCH
 }
 
 - (void)trackApplicationUIKitNotifications
@@ -129,17 +132,23 @@ - (void)trackApplicationUIKitNotifications
 #if !TARGET_OS_WATCH
 - (void)trackNetworkConnectivityChanges
 {
-    [SentryDependencyContainer.sharedInstance.reachability
-         addObserver:self
-        withCallback:^(BOOL connected, NSString *_Nonnull typeDescription) {
-            SentryBreadcrumb *crumb =
-                [[SentryBreadcrumb alloc] initWithLevel:kSentryLevelInfo
-                                               category:@"device.connectivity"];
-            crumb.type = @"connectivity";
-            crumb.data = [NSDictionary dictionaryWithObject:typeDescription forKey:@"connectivity"];
-            [self.delegate addBreadcrumb:crumb];
-        }];
+    [SentryDependencyContainer.sharedInstance.reachability addObserver:self];
+}
+
+- (void)stopTrackNetworkConnectivityChanges
+{
+    [SentryDependencyContainer.sharedInstance.reachability removeObserver:self];
+}
+
+- (void)connectivityChanged:(BOOL)connected typeDescription:(nonnull NSString *)typeDescription
+{
+    SentryBreadcrumb *crumb = [[SentryBreadcrumb alloc] initWithLevel:kSentryLevelInfo
+                                                             category:@"device.connectivity"];
+    crumb.type = @"connectivity";
+    crumb.data = [NSDictionary dictionaryWithObject:typeDescription forKey:@"connectivity"];
+    [self.delegate addBreadcrumb:crumb];
 }
+
 #endif // !TARGET_OS_WATCH
 
 - (void)addBreadcrumbWithType:(NSString *)type

diff --git a/Sources/Sentry/SentryHttpTransport.m b/Sources/Sentry/SentryHttpTransport.m
@@ -92,28 +92,23 @@ - (id)initWithOptions:(SentryOptions *)options
         [self sendAllCachedEnvelopes];
 
 #if !TARGET_OS_WATCH
-        __weak SentryHttpTransport *weakSelf = self;
-        [SentryDependencyContainer.sharedInstance.reachability
-             addObserver:self
-            withCallback:^(BOOL connected, NSString *_Nonnull typeDescription) {
-                if (weakSelf == nil) {
-                    SENTRY_LOG_DEBUG(@"WeakSelf is nil. Not doing anything.");
-                    return;
-                }
-
-                if (connected) {
-                    SENTRY_LOG_DEBUG(@"Internet connection is back.");
-                    [weakSelf sendAllCachedEnvelopes];
-                } else {
-                    SENTRY_LOG_DEBUG(@"Lost internet connection.");
-                }
-            }];
+        [SentryDependencyContainer.sharedInstance.reachability addObserver:self];
 #endif // !TARGET_OS_WATCH
     }
     return self;
 }
 
 #if !TARGET_OS_WATCH
+- (void)connectivityChanged:(BOOL)connected typeDescription:(nonnull NSString *)typeDescription
+{
+    if (connected) {
+        SENTRY_LOG_DEBUG(@"Internet connection is back.");
+        [self sendAllCachedEnvelopes];
+    } else {
+        SENTRY_LOG_DEBUG(@"Lost internet connection.");
+    }
+}
+
 - (void)dealloc
 {
     [SentryDependencyContainer.sharedInstance.reachability removeObserver:self];

diff --git a/Sources/Sentry/SentryReachability.m b/Sources/Sentry/SentryReachability.m
@@ -31,10 +31,10 @@
 
 static const SCNetworkReachabilityFlags kSCNetworkReachabilityFlagsUninitialized = UINT32_MAX;
 
-static NSMutableDictionary<NSString *, SentryConnectivityChangeBlock>
-    *sentry_reachability_change_blocks;
+static NSHashTable<id<SentryReachabilityObserver>> *sentry_reachability_observers;
 static SCNetworkReachabilityFlags sentry_current_reachability_state
     = kSCNetworkReachabilityFlagsUninitialized;
+static dispatch_queue_t sentry_reachability_queue;
 
 NSString *const SentryConnectivityCellular = @"cellular";
 NSString *const SentryConnectivityWiFi = @"wifi";
@@ -58,6 +58,9 @@
     // Check if the reported state is different from the last known state (if any)
     SCNetworkReachabilityFlags newFlags = flags & importantFlags;
     if (newFlags == sentry_current_reachability_state) {
+        SENTRY_LOG_DEBUG(@"No change in reachability state. SentryConnectivityShouldReportChange "
+                         @"will return NO for flags %u, sentry_current_reachability_state %u",
+            flags, sentry_current_reachability_state);
         return NO;
     }
 
@@ -89,19 +92,42 @@
 SentryConnectivityCallback(
     __unused SCNetworkReachabilityRef target, SCNetworkReachabilityFlags flags, __unused void *info)
 {
-    if (sentry_reachability_change_blocks && SentryConnectivityShouldReportChange(flags)) {
+    SENTRY_LOG_DEBUG(
+        @"SentryConnectivityCallback called with target: %@; flags: %u", target, flags);
+
+    @synchronized(sentry_reachability_observers) {
+        SENTRY_LOG_DEBUG(
+            @"Entered synchronized region of SentryConnectivityCallback with target: %@; flags: %u",
+            target, flags);
+
+        if (sentry_reachability_observers.count == 0) {
+            SENTRY_LOG_DEBUG(@"No reachability observers registered. Nothing to do.");
+            return;
+        }
+
+        if (!SentryConnectivityShouldReportChange(flags)) {
+            SENTRY_LOG_DEBUG(@"SentryConnectivityShouldReportChange returned NO for flags %u, will "
+                             @"not report change to observers.",
+                flags);
+            return;
+        }
+
         BOOL connected = (flags & kSCNetworkReachabilityFlagsReachable) != 0;
 
-        for (SentryConnectivityChangeBlock block in sentry_reachability_change_blocks.allValues) {
-            block(connected, SentryConnectivityFlagRepresentation(flags));
+        SENTRY_LOG_DEBUG(@"Notifying observers...");
+        for (id<SentryReachabilityObserver> observer in sentry_reachability_observers) {
+            SENTRY_LOG_DEBUG(@"Notifying %@", observer);
+            [observer connectivityChanged:connected
+                          typeDescription:SentryConnectivityFlagRepresentation(flags)];
         }
+        SENTRY_LOG_DEBUG(@"Finished notifying observers.");
     }
 }
 
 void
 SentryConnectivityReset(void)
 {
-    [sentry_reachability_change_blocks removeAllObjects];
+    [sentry_reachability_observers removeAllObjects];
     sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
 }
 
@@ -110,64 +136,99 @@ @implementation SentryReachability
 + (void)initialize
 {
     if (self == [SentryReachability class]) {
-        sentry_reachability_change_blocks = [NSMutableDictionary new];
+        sentry_reachability_observers = [NSHashTable weakObjectsHashTable];
     }
 }
 
-- (void)dealloc
+- (instancetype)init
 {
-    sentry_reachability_change_blocks = [NSMutableDictionary dictionary];
-    [self removeReachabilityNotification];
+    if (self = [super init]) {
+        self.setReachabilityCallback = YES;
+    }
+
+    return self;
 }
 
-- (void)addObserver:(id<SentryReachabilityObserver>)observer
-       withCallback:(SentryConnectivityChangeBlock)block;
+- (void)addObserver:(id<SentryReachabilityObserver>)observer;
 {
-    sentry_reachability_change_blocks[[observer description]] = block;
+    SENTRY_LOG_DEBUG(@"Adding observer: %@", observer);
+    @synchronized(sentry_reachability_observers) {
+        SENTRY_LOG_DEBUG(@"Synchronized to add observer: %@", observer);
+        if ([sentry_reachability_observers containsObject:observer]) {
+            SENTRY_LOG_DEBUG(@"Observer already added. Doing nothing.");
+            return;
+        }
 
-    if (sentry_current_reachability_state != kSCNetworkReachabilityFlagsUninitialized) {
-        return;
-    }
+        [sentry_reachability_observers addObject:observer];
+
+        if (sentry_reachability_observers.count > 1) {
+            return;
+        }
+
+        if (!self.setReachabilityCallback) {
+            SENTRY_LOG_DEBUG(@"Skipping setting reachability callback.");
+            return;
+        }
 
-    static dispatch_once_t once_t;
-    static dispatch_queue_t reachabilityQueue;
-    dispatch_once(&once_t, ^{
-        reachabilityQueue
+        sentry_reachability_queue
             = dispatch_queue_create("io.sentry.cocoa.connectivity", DISPATCH_QUEUE_SERIAL);
-    });
+        _sentry_reachability_ref = SCNetworkReachabilityCreateWithName(NULL, "sentry.io");
+        if (!_sentry_reachability_ref) { // Can be null if a bad hostname was specified
+            return;
+        }
 
-    _sentry_reachability_ref = SCNetworkReachabilityCreateWithName(NULL, "sentry.io");
-    if (!_sentry_reachability_ref) { // Can be null if a bad hostname was specified
-        return;
+        SENTRY_LOG_DEBUG(@"registering callback for reachability ref %@", _sentry_reachability_ref);
+        SCNetworkReachabilitySetCallback(
+            _sentry_reachability_ref, SentryConnectivityCallback, NULL);
+        SCNetworkReachabilitySetDispatchQueue(_sentry_reachability_ref, sentry_reachability_queue);
     }
-
-    SENTRY_LOG_DEBUG(@"registering callback for reachability ref %@", _sentry_reachability_ref);
-    SCNetworkReachabilitySetCallback(_sentry_reachability_ref, SentryConnectivityCallback, NULL);
-    SCNetworkReachabilitySetDispatchQueue(_sentry_reachability_ref, reachabilityQueue);
 }
 
 - (void)removeObserver:(id<SentryReachabilityObserver>)observer
 {
-    [sentry_reachability_change_blocks removeObjectForKey:[observer description]];
-    if (sentry_reachability_change_blocks.allValues.count > 0) {
-        return;
+    SENTRY_LOG_DEBUG(@"Removing observer: %@", observer);
+    @synchronized(sentry_reachability_observers) {
+        SENTRY_LOG_DEBUG(@"Synchronized to remove observer: %@", observer);
+        [sentry_reachability_observers removeObject:observer];
+
+        [self unsetReachabilityCallbackIfNeeded];
     }
+}
 
-    [self removeReachabilityNotification];
+- (void)removeAllObservers
+{
+    SENTRY_LOG_DEBUG(@"Removing all observers.");
+    @synchronized(sentry_reachability_observers) {
+        SENTRY_LOG_DEBUG(@"Synchronized to remove all observers.");
+        [sentry_reachability_observers removeAllObjects];
+        [self unsetReachabilityCallbackIfNeeded];
+    }
 }
 
-- (void)removeReachabilityNotification
+- (void)unsetReachabilityCallbackIfNeeded
 {
-    sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
+    if (sentry_reachability_observers.count > 0) {
+        SENTRY_LOG_DEBUG(
+            @"Other observers still registered, will not unset reachability callback.");
+        return;
+    }
 
-    if (_sentry_reachability_ref == nil) {
-        SENTRY_LOG_WARN(@"No reachability ref to unregister.");
+    if (!self.setReachabilityCallback) {
+        SENTRY_LOG_DEBUG(@"Skipping unsetting reachability callback.");
         return;
     }
 
-    SENTRY_LOG_DEBUG(@"removing callback for reachability ref %@", _sentry_reachability_ref);
-    SCNetworkReachabilitySetCallback(_sentry_reachability_ref, NULL, NULL);
-    SCNetworkReachabilitySetDispatchQueue(_sentry_reachability_ref, NULL);
+    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);
+        _sentry_reachability_ref = nil;
+    }
+
+    SENTRY_LOG_DEBUG(@"Cleaning up reachability queue.");
+    sentry_reachability_queue = nil;
 }
 
 @end

diff --git a/Sources/Sentry/include/SentryReachability.h b/Sources/Sentry/include/SentryReachability.h
@@ -43,14 +43,16 @@ SENTRY_EXTERN NSString *const SentryConnectivityCellular;
 SENTRY_EXTERN NSString *const SentryConnectivityWiFi;
 SENTRY_EXTERN NSString *const SentryConnectivityNone;
 
+@protocol SentryReachabilityObserver <NSObject>
+
 /**
- * Function signature to connectivity monitoring callback of @c SentryReachability
+ * Called when network connectivity changes.
+ *
  * @param connected @c YES if the monitored URL is reachable
  * @param typeDescription a textual representation of the connection type
  */
-typedef void (^SentryConnectivityChangeBlock)(BOOL connected, NSString *typeDescription);
+- (void)connectivityChanged:(BOOL)connected typeDescription:(NSString *)typeDescription;
 
-@protocol SentryReachabilityObserver <NSObject>
 @end
 
 /**
@@ -60,17 +62,22 @@ typedef void (^SentryConnectivityChangeBlock)(BOOL connected, NSString *typeDesc
 @interface SentryReachability : NSObject
 
 /**
- * Invoke a block each time network connectivity changes
- * @param block The block called when connectivity changes
+ * Only needed for testing.
  */
-- (void)addObserver:(id<SentryReachabilityObserver>)observer
-       withCallback:(SentryConnectivityChangeBlock)block;
+@property (nonatomic, assign) BOOL setReachabilityCallback;
+
+/**
+ * Add an observer which is called each time network connectivity changes.
+ */
+- (void)addObserver:(id<SentryReachabilityObserver>)observer;
 
 /**
  * Stop monitoring the URL previously configured with @c monitorURL:usingCallback:
  */
 - (void)removeObserver:(id<SentryReachabilityObserver>)observer;
 
+- (void)removeAllObservers;
+
 @end
 
 NS_ASSUME_NONNULL_END