Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Multiple issues in SentryReachability #3338

Merged
merged 14 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- App hang with race condition for tick counter (#3290)
- Remove "duplicate library" warning (#3312)
- Remove unnecessaries build settings (#3325)
- Fix multiple issues in Reachability (#3338)

## 8.13.0

Expand Down
4 changes: 4 additions & 0 deletions Sentry.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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>"; };
Expand Down Expand Up @@ -2742,6 +2744,7 @@
7B5CAF7C27F5AD0600ED0DB6 /* TestNSURLRequestBuilder.h */,
7B5CAF7D27F5AD3500ED0DB6 /* TestNSURLRequestBuilder.m */,
0AE455AC28F584D2006680E5 /* SentryReachabilityTests.m */,
629690522AD3E060000185FA /* SentryReachabilitySwiftTests.swift */,
0A9415B928F96CAC006A5DD1 /* TestSentryReachability.swift */,
);
path = Networking;
Expand Down Expand Up @@ -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 */,
Expand Down
7 changes: 6 additions & 1 deletion SentryTestUtils/ClearTestState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
29 changes: 19 additions & 10 deletions Sources/Sentry/SentryBreadcrumbTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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];
Copy link
Member

@armcknight armcknight Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'd made this self.delegate a weak self instead, would we have broken a block capture causing issues here? Just trying to understand why this works if you break use the delegate callback pattern instead of ObjC block callback.

ETA: I believe NSHashTable uses -[NSObject hash] as the key for the objects; couldn't we use the NSDictionary with the hashes as keys instead of the problematic descriptions? Moot point I guess since you've done the work, but if that would work we might consider doing that to minimize git history churn. We could swap out NSDictionary for NSMapTable to get the weak object semantics.

Copy link
Member Author

@philipphofmann philipphofmann Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self.delegate is already weak reference see

@property (nonatomic, weak) id<SentryBreadcrumbDelegate> delegate;
so we don't need to make it a weak self.

I personally prefer the delegate pattern, cause you don't need to worry about using weak references in all the block if you set it up properly. Otherwise, you have to ensure using weak references in the blocks which is easier to get wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't we use the NSDictionary with the hashes as keys instead of the problematic descriptions

Yes, if we use NSMapTable as pointed out by you, but I would prefer the NSHashTable, cause we don't have to worry about how to come up with the keys and write .allValues two times. I can change it if you prefer. No strong opinion on this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'm fine with all this!

}];
[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
Expand Down
27 changes: 11 additions & 16 deletions Sources/Sentry/SentryHttpTransport.m
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
84 changes: 53 additions & 31 deletions Sources/Sentry/SentryReachability.m
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -89,11 +89,14 @@
SentryConnectivityCallback(
__unused SCNetworkReachabilityRef target, SCNetworkReachabilityFlags flags, __unused void *info)
{
if (sentry_reachability_change_blocks && SentryConnectivityShouldReportChange(flags)) {
BOOL connected = (flags & kSCNetworkReachabilityFlagsReachable) != 0;

for (SentryConnectivityChangeBlock block in sentry_reachability_change_blocks.allValues) {
block(connected, SentryConnectivityFlagRepresentation(flags));
@synchronized(sentry_reachability_observers) {
if (sentry_reachability_observers && SentryConnectivityShouldReportChange(flags)) {
BOOL connected = (flags & kSCNetworkReachabilityFlagsReachable) != 0;

for (id<SentryReachabilityObserver> observer in sentry_reachability_observers) {
[observer connectivityChanged:connected
typeDescription:SentryConnectivityFlagRepresentation(flags)];
}
}
}
}
Expand All @@ -103,47 +106,64 @@
+ (void)initialize
{
if (self == [SentryReachability class]) {
sentry_reachability_change_blocks = [NSMutableDictionary new];
sentry_reachability_observers = [NSHashTable weakObjectsHashTable];
}
}

- (void)dealloc
- (instancetype)init
{
for (id<SentryReachabilityObserver> observer in sentry_reachability_change_blocks.allKeys) {
[self removeObserver:observer];
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;
@synchronized(sentry_reachability_observers) {
armcknight marked this conversation as resolved.
Show resolved Hide resolved
[sentry_reachability_observers addObject:observer];

if (sentry_current_reachability_state != kSCNetworkReachabilityFlagsUninitialized) {
return;
}
if (sentry_reachability_observers.count > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we remove all observers, and then add one again? It will reenter the logic below, is that desired? I still think this would be better with a dispatch_once to avoid that situation and also it may just be more optimized than calling objc_msgSend for count and comparing the integer value.

ETA: I see how it's cleaned up after reading below. I would say at a minimum leave a comment explaining here, but ideally we'd have some more semantic code wrapping init/teardown. You already have the teardown wrapped, but the init is a bit unclear here. In general though I like your solution best because we release the memory for the unused queue and stop callbacks from SCNetworkReachabilitySetCallback being sent when there are no observers. Nice work 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dispatch once caused some problems in the tests. Yes, I want to properly teardown everything. I will include your feedback. Thanks 🙏

Copy link
Member

@armcknight armcknight Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that is another good point 👍🏻 guess we need a SentryDispatchOnce wrapper we can subclass for tests 😆

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;

Check warning on line 135 in Sources/Sentry/SentryReachability.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryReachability.m#L135

Added line #L135 was not covered by tests
}

_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);
if (self.setReachabilityCallback) {
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) {
@synchronized(sentry_reachability_observers) {
[sentry_reachability_observers removeObject:observer];

[self unsetCallbackIfNeeded];
}
}

- (void)removeAllObservers
{
@synchronized(sentry_reachability_observers) {
armcknight marked this conversation as resolved.
Show resolved Hide resolved
[sentry_reachability_observers removeAllObjects];
[self unsetCallbackIfNeeded];
}
}

- (void)unsetCallbackIfNeeded
{
if (sentry_reachability_observers.count > 0) {
return;
}

Expand All @@ -157,6 +177,8 @@
SENTRY_LOG_DEBUG(@"removing callback for reachability ref %@", _sentry_reachability_ref);
SCNetworkReachabilitySetCallback(_sentry_reachability_ref, NULL, NULL);
SCNetworkReachabilitySetDispatchQueue(_sentry_reachability_ref, NULL);
sentry_reachability_queue = nil;
_sentry_reachability_ref = nil;
}

@end
Expand Down
21 changes: 14 additions & 7 deletions Sources/Sentry/include/SentryReachability.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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) 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@

func testNetworkConnectivityChangeBreadcrumbs() throws {
let testReachability = TestSentryReachability()

SentryDependencyContainer.sharedInstance().reachability = testReachability

let sut = SentryBreadcrumbTracker()
sut.start(with: delegate)
let states = [SentryConnectivityCellular,
Expand All @@ -55,6 +57,15 @@
}

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)
Expand All @@ -74,12 +85,12 @@
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)")

Check warning on line 89 in Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift

View check run for this annotation

Codecov / codecov/patch

Tests/SentryTests/Integrations/Breadcrumbs/SentryBreadcrumbTrackerTests.swift#L89

Added line #L89 was not covered by tests
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,10 @@ class SentryHttpTransportTests: XCTestCase {
}

func testDealloc_StopsReachabilityMonitoring() {
_ = fixture.sut
func deallocSut() {
_ = fixture.sut
}
deallocSut()

XCTAssertEqual(1, fixture.reachability.stopMonitoringInvocations.count)
}
Expand Down
20 changes: 20 additions & 0 deletions Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import XCTest

final class SentryReachabilitySwiftTests: XCTestCase, SentryReachabilityObserver {

func testAddRemoveFromMultipleThreads() throws {
let sut = SentryReachability()
testConcurrentModifications(writeWork: {_ in
sut.add(self)
}, readWork: {
sut.remove(self)
})

sut.removeAllObservers()
}

func connectivityChanged(_ connected: Bool, typeDescription: String) {
// Do nothing
}

Check warning on line 18 in Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift

View check run for this annotation

Codecov / codecov/patch

Tests/SentryTests/Networking/SentryReachabilitySwiftTests.swift#L17-L18

Added lines #L17 - L18 were not covered by tests

}
Loading
Loading