This repository has been archived by the owner on Jan 17, 2023. It is now read-only.
Fixed race condition in reachability callback delivery #3117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because the run-loop reachability callback executed its callback directly, whereas the initial check (fired off from `-[AFNetworkReachabilityManager startMonitoring]`) executed the callback from a GCD queue, it was possible for the callbacks to be executed out of order from when the status was obtained, leaving the listener in the incorrect state. By sending both routes through the same GCD-queuing function, both the callbacks and NSNotifications will be processed in the order in which they are sent. It also removes some repetition in the code.
It appears that a check was put in place to work around a race condition when reachability was associated with a domain name. However, the cause of the race condition is addressed in the previous commit, so the special-casing is no longer needed.
|
This looks great! Thanks for putting this together. I should be able to get this merged in today, and I plan on shipping it in the 2.6.2 release. |
kcharwood
changed the title
kcharwood
changed the title
|
I appreciate the comment! I'm glad it's of use. |
kcharwood
added a commit
that referenced
this pull request
Nov 6, 2015
…allbacks Fixed race condition in reachability callback delivery
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative implementation of a previous attempt to fix a race condition in reachability notifications, particularly with domain-name-based associations. However, we encountered cases where the status-change callback could still receive notifications out of order, due to one path executing the callback immediately on the runloop thread and the other queuing the callback onto the GCD main queue (which is serviced by the same run loop, but the order in which queue blocks and other run loop callbacks are executed is not easy to determine).
I broke this into two commits: The first is the fix for the issue, by sending all callback notifications through the GCD main queue, just as the NSNotifications were being sent. This is the code we've been using for a few weeks and it has resolved all our issues. The second removes the special case for domain-based reachability associations, as I don't think there's any need for it with this implementation. However, I have not tested this; I just noticed that there was an attempt to remove it a few months ago, but it was quickly restored due to reintroducing the race condition. If I've really solved the root cause, then it should be safe to remove this now. But I can't think of a way to unit-test this, so it needs to be tried in a few different apps to see if it's safe. If not, I can reverse that commit and resubmit with the first alone.