Merge pull request #10570 from Artemych:fix/progressive_downloader_in…

…finite_loop PiperOrigin-RevId: 472475124
google · Oct 19, 2022 · 9c56b2c · 9c56b2c
2 parents 6e45824 + dec01c8
commit 9c56b2c
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 15 deletions.
diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java
@@ -15,6 +15,8 @@
  */
 package com.google.android.exoplayer2.offline;
 
+import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
+
 import androidx.annotation.Nullable;
 import com.google.android.exoplayer2.C;
 import com.google.android.exoplayer2.MediaItem;
@@ -88,26 +90,26 @@ public ProgressiveDownloader(
   public void download(@Nullable ProgressListener progressListener)
       throws IOException, InterruptedException {
     this.progressListener = progressListener;
-    downloadRunnable =
-        new RunnableFutureTask<Void, IOException>() {
-          @Override
-          protected Void doWork() throws IOException {
-            cacheWriter.cache();
-            return null;
-          }
-
-          @Override
-          protected void cancelWork() {
-            cacheWriter.cancel();
-          }
-        };
-
     if (priorityTaskManager != null) {
       priorityTaskManager.add(C.PRIORITY_DOWNLOAD);
     }
     try {
       boolean finished = false;
       while (!finished && !isCanceled) {
+        // Recreate downloadRunnable on each loop iteration to avoid rethrowing a previous error.
+        downloadRunnable =
+            new RunnableFutureTask<Void, IOException>() {
+              @Override
+              protected Void doWork() throws IOException {
+                cacheWriter.cache();
+                return null;
+              }
+
+              @Override
+              protected void cancelWork() {
+                cacheWriter.cancel();
+              }
+            };
         if (priorityTaskManager != null) {
           priorityTaskManager.proceed(C.PRIORITY_DOWNLOAD);
         }
@@ -130,7 +132,7 @@ protected void cancelWork() {
     } finally {
       // If the main download thread was interrupted as part of cancelation, then it's possible that
       // the runnable is still doing work. We need to wait until it's finished before returning.
-      downloadRunnable.blockUntilFinished();
+      checkNotNull(downloadRunnable).blockUntilFinished();
       if (priorityTaskManager != null) {
         priorityTaskManager.remove(C.PRIORITY_DOWNLOAD);
       }

diff --git a/...y/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java b/...y/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java
@@ -21,6 +21,7 @@
 import android.net.Uri;
 import androidx.test.core.app.ApplicationProvider;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
+import com.google.android.exoplayer2.C;
 import com.google.android.exoplayer2.MediaItem;
 import com.google.android.exoplayer2.database.DatabaseProvider;
 import com.google.android.exoplayer2.testutil.FailOnCloseDataSink;
@@ -32,6 +33,7 @@
 import com.google.android.exoplayer2.upstream.cache.CacheDataSource;
 import com.google.android.exoplayer2.upstream.cache.NoOpCacheEvictor;
 import com.google.android.exoplayer2.upstream.cache.SimpleCache;
+import com.google.android.exoplayer2.util.PriorityTaskManager;
 import com.google.android.exoplayer2.util.Util;
 import java.io.File;
 import java.io.IOException;
@@ -126,6 +128,51 @@ public void download_afterWriteFailureOnClose_succeeds() throws Exception {
     assertThat(progressListener.bytesDownloaded).isEqualTo(1024);
   }
 
+  @Test
+  public void download_afterPriorityTooLow_succeeds() throws Exception {
+    PriorityTaskManager priorityTaskManager = new PriorityTaskManager();
+    AtomicBoolean hasSetPlaybackPriority = new AtomicBoolean(/* initialValue= */ false);
+    Uri uri = Uri.parse("test:///test.mp4");
+    // Fake data which briefly sets the playback priority while downloading for the first time.
+    FakeDataSet data = new FakeDataSet();
+    data.newData(uri)
+        .appendReadAction(
+            () -> {
+              if (!hasSetPlaybackPriority.getAndSet(true)) {
+                // This only interrupts the download the next time the DataSource checks for the
+                // priority, so delay the removal of the playback priority slightly. As we can't
+                // check when the DataSource throws the PriorityTooLowException exactly, we need to
+                // use an arbitrary delay.
+                priorityTaskManager.add(C.PRIORITY_PLAYBACK);
+                new Thread(
+                        () -> {
+                          try {
+                            Thread.sleep(200);
+                          } catch (InterruptedException e) {
+                            // Ignore.
+                          }
+                          priorityTaskManager.remove(C.PRIORITY_PLAYBACK);
+                        })
+                    .start();
+              }
+            })
+        .appendReadData(2_000_000);
+    DataSource.Factory upstreamDataSource = new FakeDataSource.Factory().setFakeDataSet(data);
+    MediaItem mediaItem = MediaItem.fromUri(uri);
+    CacheDataSource.Factory cacheDataSourceFactory =
+        new CacheDataSource.Factory()
+            .setCache(downloadCache)
+            .setUpstreamDataSourceFactory(upstreamDataSource)
+            .setUpstreamPriorityTaskManager(priorityTaskManager);
+    ProgressiveDownloader downloader = new ProgressiveDownloader(mediaItem, cacheDataSourceFactory);
+    TestProgressListener progressListener = new TestProgressListener();
+
+    // Download expected to finish (despite the interruption by the higher playback priority).
+    downloader.download(progressListener);
+
+    assertThat(progressListener.bytesDownloaded).isEqualTo(2_000_000);
+  }
+
   private static final class TestProgressListener implements Downloader.ProgressListener {
 
     public long bytesDownloaded;