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

Download RequirementWatcher misses network transition #6733

Closed
szaboa opened this issue Dec 6, 2019 · 6 comments
Closed

Download RequirementWatcher misses network transition #6733

szaboa opened this issue Dec 6, 2019 · 6 comments
Assignees
Labels

Comments

@szaboa
Copy link
Contributor

szaboa commented Dec 6, 2019

[REQUIRED] Issue description

The RequirementWatcher of downloads sometimes misses to detect that a network is available and because of this the download will go back to queue incorrectly.

[REQUIRED] Reproduction steps

The issue can be reproduced with ExoPlayer's demo app. The default requirement of downloads in the demo app is com.google.android.exoplayer2.scheduler.Requirements#NETWORK, which should allow to download on Wifi and cellular network.

  1. Be connected to Wifi and also enable LTE.
  2. Start to download any content in the demo app.
  3. Wait until it starts and the progress can be seen in the notification.
  4. Disable WiFi.

Expected result:
The phone switches to mobile data and the download continues.

Actual result:
The phone switches to mobile data but the download stops (goes into queue and reporting that the network requirement is not met).

Note: this behavior can be seen only approx. 7/10 times, sometimes works as expected.

[REQUIRED] Link to test content

Any content in the demo app.

[REQUIRED] A full bug report captured from the device

N/A

[REQUIRED] Version of ExoPlayer being used

Version 2.10.2, but also branch dev-v2.

[REQUIRED] Device(s) and version(s) of Android being used

  • Pixel 1 - physical device
  • Pixel 3xl - emulator

Additional info:

The root cause seems to be that after turning off the Wifi RequirementsWatcher.CapabilityValidatedCallback#onLost is called correctly but RequirementsWatcher.CapabilityValidatedCallback#onAvailable is not called when goes over to mobile network.

Using connectivityManager.registerDefaultNetworkCallback(networkCallback) instead of connectivityManager.registerNetworkCallback(request, networkCallback) solves the problem (at least in my tests it worked 10/10) but then we will miss the internet connectivity validation (same when API < 23).

@ojw28
Copy link
Contributor

ojw28 commented Dec 20, 2019

RequirementsWatcher.CapabilityValidatedCallback#onAvailable is not called when goes over to mobile network.

I think this is working as intended, because both WiFi and the LTE network are already available. The problem is that when we see the WiFi network's onLost being called and go to update the Requirements, the ConnectivityManager still think that the WiFi network is the current network. It's not disconnected, so we conclude that the current network doesn't have connectivity. We then don't receive another event when the current network changes.

I think switching to registerDefaultNetworkCallback for API levels >= 24 is the right thing to do. It doesn't matter that the callback isn't registered with the NET_CAPABILITY_VALIDATED capability. Note that RequirementsWatcher.getNotMetNetworkRequirements evaluates the capability anyway.

@ojw28 ojw28 assigned ojw28 and unassigned marcbaechinger Dec 20, 2019
@szaboa
Copy link
Contributor Author

szaboa commented Dec 20, 2019

I see. Then let's use registerDefaultNetworkCallback for API levels >= 24 and the broadcast receiver for API levels < 24?

Is this open for contributions? I can prepare a PR :)

@ojw28
Copy link
Contributor

ojw28 commented Dec 20, 2019

It would be except that I already have one ;).

@szaboa
Copy link
Contributor Author

szaboa commented Dec 20, 2019

Nice 👍

ojw28 added a commit that referenced this issue Dec 20, 2019
@ojw28
Copy link
Contributor

ojw28 commented Dec 20, 2019

Please see the patch referenced above. It would be great if you could test it out on the dev-v2 branch, preferably before we include it in a release, because it's somewhat hard to be certain we've done the right thing!

@ojw28 ojw28 closed this as completed Dec 20, 2019
@szaboa
Copy link
Contributor Author

szaboa commented Dec 21, 2019

I've just tested it on my Pixel, it worked 10/10 :)

ojw28 added a commit that referenced this issue Jan 17, 2020
@google google locked and limited conversation to collaborators Feb 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants