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

Refactor Reachability #2915

Merged
merged 4 commits into from Aug 25, 2019

Conversation

@jshier
Copy link
Contributor

commented Aug 21, 2019

Goals ⚽️

This PR refactors our SCNetworkReachability wrapper to simplify usage, adopt more modern styling, and fix a bug.

Implementation Details 🚧

There are a few changes in here:

  • Moved ConnectionType inside NetworkReachabilityStatus, since the types are related.
  • Added default static instance (using the zero address) so users don't have to create an instance in the common case.
  • Renamed usages of WWAN to Cellular, since that's what it actually means.
  • Rewrote DispatchQueue handling to separate the queue used for reachability notifications and listener callbacks. Notifications are now always on a background queue and the listener queue is now customizable when listening is started, while defaulting to .main.
  • Lifetime management has been updated: startListening now calls stopListening to clear any state before setting up a new SCNetworkReachabilityContext.
  • The zero address initializer has been simplified to no longer require memory rebounding.
  • Various status computations have been moved to initializers or convenience properties, rather than methods on the manager itself.
  • Added support for the cellular checks on tvOS because they're technically supported, so why not.

Testing Details 🔍

Added some lifecycle tests where the manager is started, stopped, and started again, and updated other tests that calculate the reachability status.

@cnoon
cnoon approved these changes Aug 22, 2019
Copy link
Member

left a comment

Looks good to me @jshier. A couple comments back your way for you to noodle on.

/// A closure executed when the network reachability status changes.
open var listener: Listener?
/// `DispatchQueue` on which reachability will update.
public let reachabilityQueue = DispatchQueue(label: "org.alamofire.reachabilityQueue")

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 22, 2019

Member

Does the label become an issue if you create more than 1 manager? I'm sure I've done this before without realizing. Would two separate DispatchQueues created with the same label actually be two different instances that just happened to share the same label? Would that only affect the queue visualization in Xcode? Or are there other implications?

This comment has been minimized.

Copy link
@jshier

jshier Aug 22, 2019

Author Contributor

It's just a debugging aid, so it would only affect visualization in the debugger and Xcode. If we want to ensure all the queues we use are uniquely labeled, I think we should do a separate PR, as there are a few queues in the Session that would need to be made unique. I'm not sure how valuable it is, since you can always tell queues apart by their address.

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 22, 2019

Member

Okay, thanks for confirming. That was my guess as to how it worked. I don't know how often users would have two sessions or managers. It would also be difficult to know which one was which. Probably not that useful.

Source/NetworkReachabilityManager.swift Show resolved Hide resolved
Source/NetworkReachabilityManager.swift Outdated Show resolved Hide resolved
@cnoon
cnoon approved these changes Aug 22, 2019
Copy link
Member

left a comment

👌🏻

@jshier jshier merged commit 052c2c2 into master Aug 25, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier deleted the feature/reachability-refactor branch Aug 25, 2019

@jshier jshier added this to the 5.0.0.rc.1 milestone Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.