From 1437c68799905a372ab56d78857ecd1ec4545081 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Wed, 11 Oct 2023 22:43:53 +0200 Subject: [PATCH] fix: Multiple issues in SentryReachability (#3338) --- CHANGELOG.md | 1 + Sentry.xcodeproj/project.pbxproj | 4 + SentryTestUtils/ClearTestState.swift | 7 +- .../SentryTestUtils-ObjC-BridgingHeader.h | 1 + Sources/Sentry/SentryBreadcrumbTracker.m | 29 ++-- Sources/Sentry/SentryHttpTransport.m | 27 ++-- Sources/Sentry/SentryReachability.m | 139 +++++++++++++----- Sources/Sentry/include/SentryReachability.h | 21 ++- .../SentryBreadcrumbTrackerTests.swift | 17 ++- .../Networking/SentryHttpTransportTests.swift | 5 +- .../SentryReachabilitySwiftTests.swift | 19 +++ .../Networking/SentryReachabilityTests.m | 120 ++++++++++++--- .../Networking/TestSentryReachability.swift | 16 +- .../SentryTests/SentryTests-Bridging-Header.h | 1 + 14 files changed, 305 insertions(+), 102 deletions(-) create mode 100644 Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 21c41a74370..160edcea216 100644 --- 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 index 72f202e9721..d500f0ddf21 100644 --- 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 = ""; }; 62885DA629E946B100554F38 /* TestConncurrentModifications.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestConncurrentModifications.swift; sourceTree = ""; }; 62950F0F29E7FE0100A42624 /* SentryTransactionContextTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryTransactionContextTests.swift; sourceTree = ""; }; + 629690522AD3E060000185FA /* SentryReachabilitySwiftTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryReachabilitySwiftTests.swift; sourceTree = ""; }; 62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryTestLogConfig.m; sourceTree = ""; }; 62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBreadcrumbDelegate.h; path = include/SentryBreadcrumbDelegate.h; sourceTree = ""; }; 62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBreadcrumbTestDelegate.swift; sourceTree = ""; }; @@ -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 index 0348f1e526c..ed217344a4d 100644 --- 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 index 63ab8d9a5da..b7aba222234 100644 --- 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 index 0f189e99468..beb206b9784 100644 --- 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 index 0ad49345c73..492484513ab 100644 --- 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 index 7c1d3398284..443b286452e 100644 --- a/Sources/Sentry/SentryReachability.m +++ b/Sources/Sentry/SentryReachability.m @@ -31,10 +31,10 @@ static const SCNetworkReachabilityFlags kSCNetworkReachabilityFlagsUninitialized = UINT32_MAX; -static NSMutableDictionary - *sentry_reachability_change_blocks; +static NSHashTable> *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 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)observer - withCallback:(SentryConnectivityChangeBlock)block; +- (void)addObserver:(id)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)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 index 029b5aaf787..b481916f150 100644 --- 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 + /** - * 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 @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)observer - withCallback:(SentryConnectivityChangeBlock)block; +@property (nonatomic, assign) BOOL setReachabilityCallback; + +/** + * Add an observer which is called each time network connectivity changes. + */ +- (void)addObserver:(id)observer; /** * Stop monitoring the URL previously configured with @c monitorURL:usingCallback: */ - (void)removeObserver:(id)observer; +- (void)removeAllObservers; + @end NS_ASSUME_NONNULL_END diff --git a/Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift b/Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift index 85b5a71b5a2..88d49a120cd 100644 --- a/Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift @@ -31,7 +31,9 @@ class SentryBreadcrumbTrackerTests: XCTestCase { func testNetworkConnectivityChangeBreadcrumbs() throws { let testReachability = TestSentryReachability() + SentryDependencyContainer.sharedInstance().reachability = testReachability + let sut = SentryBreadcrumbTracker() sut.start(with: delegate) let states = [SentryConnectivityCellular, @@ -55,6 +57,15 @@ class SentryBreadcrumbTrackerTests: XCTestCase { } func testSwizzlingStarted_ViewControllerAppears_AddsUILifeCycleBreadcrumb() { + let testReachability = TestSentryReachability() + + // We already test the network breadcrumbs in a test above. Using the `TestReachability` + // makes this test more stable, as using the real implementation sometimes leads to + // test failure, cause sometimes the dispatch queue responsible for reporting the reachability + // status takes some time and then there isn't a network breadcrumb available. This test + // doesn't validate the network breadcrumb anyways. + SentryDependencyContainer.sharedInstance().reachability = testReachability + let scope = Scope() let client = TestClient(options: Options()) let hub = TestHub(client: client, andScope: scope) @@ -74,12 +85,12 @@ class SentryBreadcrumbTrackerTests: XCTestCase { let crumbs = delegate.addCrumbInvocations.invocations // one breadcrumb for starting the tracker, one for the first reachability breadcrumb and one final one for the swizzled viewDidAppear - guard crumbs.count == 3 else { - XCTFail("Expected exactly 3 breadcrumbs, got: \(crumbs)") + guard crumbs.count == 2 else { + XCTFail("Expected exactly 2 breadcrumbs, got: \(crumbs)") return } - let lifeCycleCrumb = crumbs[2] + let lifeCycleCrumb = crumbs[1] XCTAssertEqual("navigation", lifeCycleCrumb.type) XCTAssertEqual("ui.lifecycle", lifeCycleCrumb.category) XCTAssertEqual("false", lifeCycleCrumb.data?["beingPresented"] as? String) diff --git a/Tests/SentryTests/Networking/SentryHttpTransportTests.swift b/Tests/SentryTests/Networking/SentryHttpTransportTests.swift index 9c19ef37ea4..675eb687d2f 100644 --- a/Tests/SentryTests/Networking/SentryHttpTransportTests.swift +++ b/Tests/SentryTests/Networking/SentryHttpTransportTests.swift @@ -800,7 +800,10 @@ class SentryHttpTransportTests: XCTestCase { } func testDealloc_StopsReachabilityMonitoring() { - _ = fixture.sut + func deallocSut() { + _ = fixture.sut + } + deallocSut() XCTAssertEqual(1, fixture.reachability.stopMonitoringInvocations.count) } diff --git a/Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift b/Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift new file mode 100644 index 00000000000..cb1eea13667 --- /dev/null +++ b/Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift @@ -0,0 +1,19 @@ +import XCTest + +final class SentryReachabilitySwiftTests: XCTestCase { + + func testAddRemoveFromMultipleThreads() throws { + let sut = SentryReachability() + testConcurrentModifications(writeWork: {_ in + sut.add(TestReachabilityObserver()) + }, readWork: { + sut.removeAllObservers() + }) + } +} + +class TestReachabilityObserver: NSObject, SentryReachabilityObserver { + func connectivityChanged(_ connected: Bool, typeDescription: String) { + // Do nothing + } +} diff --git a/Tests/SentryTests/Networking/SentryReachabilityTests.m b/Tests/SentryTests/Networking/SentryReachabilityTests.m index 5e729343127..92d038d820f 100644 --- a/Tests/SentryTests/Networking/SentryReachabilityTests.m +++ b/Tests/SentryTests/Networking/SentryReachabilityTests.m @@ -1,3 +1,4 @@ +#import "SentryLog.h" #import "SentryReachability+Private.h" #import "SentryReachability.h" #import @@ -5,20 +6,41 @@ void SentryConnectivityReset(void); @interface TestSentryReachabilityObserver : NSObject + +@property (strong, nonatomic) XCTestExpectation *expectation; + @end @implementation TestSentryReachabilityObserver + +- (instancetype)initWithExpectation:(XCTestExpectation *)expectation +{ + if (self = [super init]) { + self.expectation = expectation; + } + return self; +} + +- (void)connectivityChanged:(BOOL)connected typeDescription:(nonnull NSString *)typeDescription +{ + NSLog(@"Received connectivity notification: %i; type: %@", connected, typeDescription); + [self.expectation fulfill]; +} + @end #if !TARGET_OS_WATCH -@interface SentryConnectivityTest : XCTestCase +@interface SentryReachabilityTest : XCTestCase @property (strong, nonatomic) SentryReachability *reachability; @end -@implementation SentryConnectivityTest +@implementation SentryReachabilityTest - (void)setUp { self.reachability = [[SentryReachability alloc] init]; + // Disable the reachability callbacks, cause we call the callbacks manually. + // Otherwise, the reachability callbacks are called during later unrelated tests causing flakes. + self.reachability.setReachabilityCallback = NO; } - (void)tearDown @@ -49,42 +71,100 @@ - (void)testConnectivityRepresentations - (void)testMultipleReachabilityObservers { - SentryReachability *reachability = [[SentryReachability alloc] init]; - XCTestExpectation *aExp = [self expectationWithDescription: @"reachability state change for observer monitoring https://sentry.io"]; aExp.expectedFulfillmentCount = 5; - TestSentryReachabilityObserver *a = [[TestSentryReachabilityObserver alloc] init]; - [reachability addObserver:a - withCallback:^(__unused BOOL connected, - NSString *_Nonnull __unused typeDescription) { [aExp fulfill]; }]; + TestSentryReachabilityObserver *a = + [[TestSentryReachabilityObserver alloc] initWithExpectation:aExp]; + [self.reachability addObserver:a]; - SentryConnectivityCallback(reachability.sentry_reachability_ref, + SentryConnectivityCallback(self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); // ignored, as it's the first callback - SentryConnectivityCallback( - reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsInterventionRequired, nil); + 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] init]; - [reachability addObserver:b - withCallback:^(__unused BOOL connected, - NSString *_Nonnull __unused typeDescription) { [bExp fulfill]; }]; + TestSentryReachabilityObserver *b = + [[TestSentryReachabilityObserver alloc] initWithExpectation:bExp]; + [self.reachability addObserver:b]; SentryConnectivityCallback( - reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + SentryConnectivityCallback(self.reachability.sentry_reachability_ref, + kSCNetworkReachabilityFlagsInterventionRequired, nil); + + [self.reachability removeObserver:b]; + SentryConnectivityCallback( - reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsInterventionRequired, nil); + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + + [self waitForExpectations:@[ aExp, bExp ] timeout:1.0]; - [reachability removeObserver:b]; + [self.reachability removeObserver:a]; +} + +- (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]; SentryConnectivityCallback( - reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + + [self waitForExpectations:@[ aExp ] timeout:1.0]; + + [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]; + + SentryConnectivityCallback( + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + + [self waitForExpectations:@[ aExp ] timeout:1.0]; + + [self.reachability removeObserver:a]; +} + +- (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]; + + SentryConnectivityCallback( + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + SentryConnectivityCallback( + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + SentryConnectivityCallback( + self.reachability.sentry_reachability_ref, kSCNetworkReachabilityFlagsReachable, nil); + + [self waitForExpectations:@[ aExp ] timeout:1.0]; - [self waitForExpectationsWithTimeout:1.f handler:nil]; + [self.reachability removeObserver:a]; } @end diff --git a/Tests/SentryTests/Networking/TestSentryReachability.swift b/Tests/SentryTests/Networking/TestSentryReachability.swift index 263604c2b93..3e1084863f6 100644 --- a/Tests/SentryTests/Networking/TestSentryReachability.swift +++ b/Tests/SentryTests/Networking/TestSentryReachability.swift @@ -3,18 +3,24 @@ import SentryTestUtils class TestSentryReachability: SentryReachability { - var block: SentryConnectivityChangeBlock? + + private var observers: NSHashTable = NSHashTable.weakObjects() - override func add(_ observer: SentryReachabilityObserver, withCallback block: @escaping SentryConnectivityChangeBlock) { - self.block = block + override func add(_ observer: SentryReachabilityObserver) { + observers.add(observer) } func setReachabilityState(state: String) { - block?(state != SentryConnectivityNone, state) + for observer in observers.allObjects { + observer.connectivityChanged(state != SentryConnectivityNone, typeDescription: state) + } + } func triggerNetworkReachable() { - block?(true, SentryConnectivityWiFi) + for observer in observers.allObjects { + observer.connectivityChanged(true, typeDescription: SentryConnectivityWiFi) + } } var stopMonitoringInvocations = Invocations() diff --git a/Tests/SentryTests/SentryTests-Bridging-Header.h b/Tests/SentryTests/SentryTests-Bridging-Header.h index e655d7f5c88..bca2e15232c 100644 --- a/Tests/SentryTests/SentryTests-Bridging-Header.h +++ b/Tests/SentryTests/SentryTests-Bridging-Header.h @@ -150,6 +150,7 @@ #import "SentryRandom.h" #import "SentryRateLimitParser.h" #import "SentryRateLimits.h" +#import "SentryReachability.h" #import "SentryRetryAfterHeaderParser.h" #import "SentrySDK+Private.h" #import "SentrySDK+Tests.h"