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 12 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 @@
- 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
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
133 changes: 94 additions & 39 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 All @@ -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;
}

Expand Down Expand Up @@ -89,19 +92,40 @@
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");
armcknight marked this conversation as resolved.
Show resolved Hide resolved

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;
}

Expand All @@ -110,64 +134,95 @@
+ (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;
@synchronized(sentry_reachability_observers) {
armcknight marked this conversation as resolved.
Show resolved Hide resolved
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) {
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;
}

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;

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

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryReachability.m#L173

Added line #L173 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);
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(@"Entered synchronized region of removeObserver");
armcknight marked this conversation as resolved.
Show resolved Hide resolved
[sentry_reachability_observers removeObject:observer];

[self unsetReachabilityCallbackIfNeeded];
}
}

[self removeReachabilityNotification];
- (void)removeAllObservers
{
@synchronized(sentry_reachability_observers) {
armcknight marked this conversation as resolved.
Show resolved Hide resolved
[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
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, 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
Expand Down
Loading