Stuck downloads when changing network conditions

[daveoxley]

Issue description

DownloadService race condition causing downloads to not resume when network requirements are met after previously not being met.

The issue appears to be that DownloadManagerHelper.onDownloadChanged is called before the new service is attached to the DownloadManagerHelper. Therefore DownloadService.notifyDownloadChanged is never called and startForeground is never called on the new service. The app process is then killed after roughly 10 seconds as the scheduler has been cancelled.

Reproduction steps

Using the exoplayer app:

• Start downloading some content
• Click back to exit the app. Downloading notification should be shown.
• Turn off network. Downloading notification should be hidden.
• Wait approximately 2 minutes.
• Turn on network. Downloading notification is not reshown when it should be.

Link to test content

Tested using WV: Clear SD & HD (MP4,H264) in the sample app.

Version of ExoPlayer being used

2.10.7. Also reproduced the issue on 2.11.1.

Device(s) and version(s) of Android being used

Pixel 3XL Android 10 (QQ1A.191205.008)

[ojw28]

This is probably a duplicate of #6733. Marking as such, but please do test with the dev-v2 branch to confirm that the issue is fixed.

[daveoxley]

@ojw28 Unfortunately this issue still exists on the latest dev-v2 branch (Tested on 826083d). The below patch fixes the issue and I'll put together a better change and submit a pr later. Any chance you could reopen this issue please?

diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java
index db10517b6..8c4220c35 100644
--- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java
@@ -32,6 +32,8 @@ import com.google.android.exoplayer2.util.Assertions;
 import com.google.android.exoplayer2.util.Log;
 import com.google.android.exoplayer2.util.NotificationUtil;
 import com.google.android.exoplayer2.util.Util;
+
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 
@@ -856,6 +858,7 @@ public abstract class DownloadService extends Service {
     @Nullable private final Scheduler scheduler;
     private final Class<? extends DownloadService> serviceClass;
     @Nullable private DownloadService downloadService;
+    private final List<Download> list = new ArrayList<>();
 
     private DownloadManagerHelper(
         Context context,
@@ -877,6 +880,10 @@ public abstract class DownloadService extends Service {
     public void attachService(DownloadService downloadService) {
       Assertions.checkState(this.downloadService == null);
       this.downloadService = downloadService;
+      for (Download download : list) {
+        downloadService.notifyDownloadChanged(download);
+      }
+      list.clear();
     }
 
     public void detachService(DownloadService downloadService, boolean unschedule) {
@@ -891,6 +898,8 @@ public abstract class DownloadService extends Service {
     public void onDownloadChanged(DownloadManager downloadManager, Download download) {
       if (downloadService != null) {
         downloadService.notifyDownloadChanged(download);
+      } else {
+        list.add(download);
       }
     }
 

[ojw28]

Thanks for checking! I see what's going on now, and yes you're absolutely right.

What's happening in this case is that the DownloadManager is calling DownloadManagerHelper. onRequirementsStateChanged, which is then failing to restart the service because the process is in the background (the "wait approximately 2 minutes" part of the reproduction instructions are important, else the process is still classified as being in the foreground). DownloadManagerHelper.onDownloadChanged is then called as the DownloadManager resumes its downloads. At some point later the Scheduler then restarts the service, which does work because it starts it as a foreground service.

Your PR will cause the DownloadManagerHelper.onDownloadChanged events to be processed when the Scheduler restarts the service, but it's a relatively uncontrolled fix because there aren't particularly strong guarantees on when this will happen. There are also some related issues, such as what happens if there's a Scheduler and foregroundNotificationId == FOREGROUND_NOTIFICATION_ID_NONE. I'd like to spend a bit of time this week seeing if there's a more controlled fix, that'll also solve this type of related issue at the same time.

[ojw28]

This should be fixed in dev-v2 now. Could you give it another try and verify?

Note that there's still some additional work to be done, which should speed up resumption in the case that the process is still in memory (and also fix this case if getScheduler is implemented to return null). See the TODOs added in 51f2723 if you're interested in what's left to do.

[ojw28]

Cleanup is complete, and the changes are in dev-v2 and dev-v2-r2.11.2. The changes were fairly extensive, so I'd really appreciate it if someone has a chance to try it out.

[daveoxley]

@ojw28 I can still make it go wrong by following the original steps using the demo app. Tested dev-v2-r2.11.2 (ce1ec1d). It doesn't appear to happen as often any more but it still happens.

[ojw28]

How many times do you generally have to try for it to reproduce? I haven't managed to reproduce the problem yet (using the same version you were using).

[ojw28]

A couple of additional:

1. I know it's an annoying question (heh, sorry), but are you 100% certain you were using that build?
2. Does it make a difference whether the demo app's process is killed between the second to last and last reproduction steps? I'm wondering if you're only seeing it reproduce sometimes because it always happens in one case or the other (i.e. process is still alive vs process is killed). Just before the final reproduction step, it should be possible for you to query whether the process is still alive or not using adb shell ps -A | grep com.google.android.exoplayer2.demo, or something along those lines.

[ojw28]

It doesn't appear to happen as often any more but it still happens.

We do now have reproduction steps for one case in which downloads fail to resume when network connectivity is restored. We're tracking this using #7453.