Add observeDefaultNetwork to ImageLoader #2379
Conversation
|
@daniellevass Are you interested in adding this to For 3.x I think we're probably better off refactoring |
| @@ -50,7 +53,12 @@ private class RealNetworkObserver( | |||
| val request = NetworkRequest.Builder() | |||
| .addCapability(NET_CAPABILITY_INTERNET) | |||
| .build() | |||
| connectivityManager.registerNetworkCallback(request, networkCallback) | |||
There was a problem hiding this comment.
We don't have to also change val isAnyOnline = connectivityManager.allNetworks.any below to only consider the default network?
There was a problem hiding this comment.
Good question - it doesn't look like it.
We had been asked by Google to only make the registerNetworkCallback to registerDefaultNetworkCallback change in our application, and we could then verify network requests using db shell dumpsys connectivity | grep -i package.name. If we got network logs of type TRACK_DEFAULT it meant everything was good and using the correct network to make the requests, but if we received LISTEN type logs it was using the incorrect network. I could verify with our test app that after the change I made, we now only got TRACK_DEFAULT logs when using coil to load images.
I can share more info on how to reproduce on an automotive emulator if you're interested, as it's not straight forward!
There was a problem hiding this comment.
Hi, James here. I'm an Android Platform lead at Google for the Connectivity teams. Thanks Danielle for mentioning this change. (and putting it up). I wanted to provide some input in case it is helpful.
Regarding Network.isOnline, what is the purpose of that code? I would assume, it checks to see if any network is online by virtue of naming. However, it doesn't seem to have any connection to the passed in networkCallback in this commented block of code? I see the callback triggers isOnline, however whatever network was passed back via the callback is then ignored by usage of connectivityManager.allNetworks.
Given that, what question is isOnline trying to answer? Here are the two questions I would guess:
- Is there a network available to me in my context? I.e., is there a network available I can use.
- Is there any network available at all whether I have access to it or not? I.e., there could be a VPN available with
INTERNETcapabilities that I don't have access to and can't use, but I would still want to showisOnlineastrue.
I'm guessing it is the former. Is there a network available that I can use? If so, I will recommend additional changes, but want to confirm before doing so.
There was a problem hiding this comment.
@tech31 Yep, you're right that isOnline (and more broadly NetworkObserver) is trying to determine if there's an available network with internet that we can use. Though, reading your other comment I think we probably only want to check the default network by default. Essentially we want to know if OkHttpClient.newCall/HttpUrlConnection/HttpClient.request will fail immediately. If so, we use this info to skip querying the network and read from the disk cache.
There was a problem hiding this comment.
Ok cool. Yes, in this case, you should always rely on the default network for the reasons I mentioned above (such as being put on a VPN, etc). Otherwise what you are returned may/will not accurately cover the intended question "do I have access to the internet".
|
Ah! I didn't think to check there was a 2.x branch - apologies! We'd much prefer this in a 2.x release if that's not too much trouble! I'll close this PR and raise a new one against that branch |
| @@ -50,7 +53,12 @@ private class RealNetworkObserver( | |||
| val request = NetworkRequest.Builder() | |||
| .addCapability(NET_CAPABILITY_INTERNET) | |||
| .build() | |||
| connectivityManager.registerNetworkCallback(request, networkCallback) | |||
There was a problem hiding this comment.
Hi, James here. I'm an Android Platform lead at Google for the Connectivity teams. Thanks Danielle for mentioning this change. (and putting it up). I wanted to provide some input in case it is helpful.
Regarding Network.isOnline, what is the purpose of that code? I would assume, it checks to see if any network is online by virtue of naming. However, it doesn't seem to have any connection to the passed in networkCallback in this commented block of code? I see the callback triggers isOnline, however whatever network was passed back via the callback is then ignored by usage of connectivityManager.allNetworks.
Given that, what question is isOnline trying to answer? Here are the two questions I would guess:
- Is there a network available to me in my context? I.e., is there a network available I can use.
- Is there any network available at all whether I have access to it or not? I.e., there could be a VPN available with
INTERNETcapabilities that I don't have access to and can't use, but I would still want to showisOnlineastrue.
I'm guessing it is the former. Is there a network available that I can use? If so, I will recommend additional changes, but want to confirm before doing so.
| @@ -50,7 +53,12 @@ private class RealNetworkObserver( | |||
| val request = NetworkRequest.Builder() | |||
| .addCapability(NET_CAPABILITY_INTERNET) | |||
| .build() | |||
| connectivityManager.registerNetworkCallback(request, networkCallback) | |||
|
|
|||
| if (observeDefaultNetwork && Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | |||
There was a problem hiding this comment.
@colinrtwhite I saw in #2374 mentioned to include observeDefaultNetwork in case someone is using a custom SocketFactory. I don't think it would have an impact to the code in this file? They can bind to whatever network they want, but it looks like this code was previously interested in networks with capability INTERNET, which could be different than the network used by the app in any case (such as by a custom SocketFactory), or is this somehow connected to a passed in SocketFactory somewhere I might have missed?
The reason I am asking, is I am wondering if you even need the observeDefaultNetwork capability to begin with. The guidance is to use registerDefaultNetworkCallback whenever possible. One of the main reasons, is when you are put on a restricted default network (e.g. a VPN). If you use registerNetworkCallback, you'll never receive a callback when that happens as it will filter out all restricted networks even though you have access to them (unless you happen to have the restricted network permission which is OEM controlled and very rare). This is one example, there are others (such as OEM usage of per-app networking features in Android).
We actually plan to add clear documentation to avoid using registerNetworkCallback unless you have a very good reason to do so in the very near future to the Android developer portal to help clear things up.
Thanks.
There was a problem hiding this comment.
@tech31 Thanks for taking a look! NetworkObserver is a weak point in the codebase as I'm not too familiar with the networking APIs. Please let me know if you see other potential improvements (or feel free to submit a PR)!
in case someone is using a custom SocketFactory
It's possible for a user to provide a custom OkHttpClient to ImageLoader, which could use a custom SocketFactory that performs network requests on a non-default network (as I understand it). It's a bit more clear on the 2.x branch where Coil depends on OkHttp directly. I wanted to err on the side of not changing the default behaviour in the 2.x (stable) branch in case it could be an unexpected change for someone. Though, if you think it's worthwhile we could directly migrate to registerDefaultNetworkCallback.
For a bit of context, Coil uses the NetworkObserver to determine if the device is "offline". If true, it automatically sends all network requests to the disk cache and fails if the image isn't disk cached.
Reading through your comments it sounds like the ideal would be to use registerDefaultNetworkCallback by default and allow users to provide a custom NetworkObserver if they don't use the default network with their OkHttpClient. This change would probably be good to land in the 3.x releases (currently in alpha). What do you think?
There was a problem hiding this comment.
Reading through your comments it sounds like the ideal would be to use registerDefaultNetworkCallback by default and allow users to provide a custom NetworkObserver if they don't use the default network with their OkHttpClient. This change would probably be good to land in the 3.x releases (currently in alpha). What do you think?
Yeah, that sounds reasonable to me from an implementation perspective. Basically, if you want to allow users to customize the types of networks they care about, you would basically do what you are already doing today by using registerNetworkCallback, except that instead of newing up a NetworkRequest in NetworkObserver, you would allow users to pass in a custom NetworkRequest which would then be used. How that would make sense from an implementation detail, e.g. new method on NetworkObserver or having a custom observer would be totally up to you.
One note on the above; I'm guessing that 99%+ of your users would be fine with the default network, so you'll probably want to comment any such functionality that users should only use such methods if they really need to, otherwise to rely on the default behavior. What we see on the Android Connectivity side is usage of registerNetworkCallback unnecessarily, which can have unintended consequences. We plan to update the guidance on the Android developer portal as well in the near future saying as much ("always use the default network unless you know what you're doing and really need something else" to paraphrase).
|
You could just use the onCapabilitiesChanged method on NetworkCallback to
get this info.
…On Mon, Jul 29, 2024 at 2:55 PM Colin White ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In coil-core/src/androidMain/kotlin/coil3/util/NetworkObserver.kt
<#2379 (comment)>:
> @@ -50,7 +53,12 @@ private class RealNetworkObserver(
val request = NetworkRequest.Builder()
.addCapability(NET_CAPABILITY_INTERNET)
.build()
- connectivityManager.registerNetworkCallback(request, networkCallback)
@tech31 <https://github.com/tech31> How should we check if the default
network has NET_CAPABILITY_INTERNET? registerDefaultNetworkCallback
doesn't accept a NetworkRequest and NetworkCallback's methods all say Do
NOT call ***@***.*** #getNetworkCapabilities(Network)} or other synchronous
ConnectivityManager methods in this callback as this is prone to race
conditions, which Network.isOnline relies on. It's not clear how to check
the default network's capabilities.
—
Reply to this email directly, view it on GitHub
<#2379 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKK22ARVQL6E4F4GHWVLKW3ZO2253AVCNFSM6AAAAABLDLQLCSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBWGE3DKNZYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi,
As per the discussion in #2374 - I have added
observeDefaultNetworkto the ImageLoader.Builder, and implemented it in the NetworkObserver.Unsure if it would be better to add it into the Extras or defaults (like networkCachePolicy for example) - as this isn't something I figure many others will change - happy to take your direction if you'd like me to refactor this at all 🙇♀️
I have tested these changes fix our specific issue by using mavenLocal and a simple Android Automotive project - I can share more on this if interested.