-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Changes from all commits
9ba4576
ca1afc5
7cd6146
624e7b8
86cc216
47fb87c
19ad7fa
3b38d9a
11d6fa9
3c44b13
b22b7ba
51d8267
a656c5f
5208e1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
armcknight marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
_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) { | ||
armcknight marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 seesentry-cocoa/Sources/Sentry/SentryBreadcrumbTracker.m
Line 32 in 3b38d9a
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we use
NSMapTable
as pointed out by you, but I would prefer theNSHashTable
, 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.There was a problem hiding this comment.
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!