Fix ProgressiveDownloader retry logic

RunnableFutureTask is not reusable. Trying to reuse it meant that a
failure in one doWork() call would cause subsequent download() calls
to (a) not block until the runnable has finished executing (does not
apply when using a direct executor), and (b) throw the same failure
as thrown from the first doWork() call.

This could cause #8078 if the initial failure occurred before the
content length was resolved. Retries are not blocked on their work
completing due to (a), and the download would be marked as failed due
to (b). The work itself could then resolve the content length, which
causes the stack trace in this issue.

Issue: #8078
PiperOrigin-RevId: 343498252

RELEASENOTES.md

@@ -108,6 +108,9 @@
         ([#7992](https://github.com/google/ExoPlayer/issues/7992)).
     *   FLV: Make files seekable by using the key frame index
         ([#7378](https://github.com/google/ExoPlayer/issues/7378)).
+*   Downloads: Fix issue retrying progressive downloads, which could also result
+    in a crash in `DownloadManager.InternalHandler.onContentLengthChanged`
+    ([#8078](https://github.com/google/ExoPlayer/issues/8078).
 *   HLS: Fix crash affecting chunkful preparation of master playlists that start
     with an I-FRAME only variant
     ([#8025](https://github.com/google/ExoPlayer/issues/8025)).

library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java

@@ -38,6 +38,7 @@ public final class ProgressiveDownloader implements Downloader {
   private final Executor executor;
   private final DataSpec dataSpec;
   private final CacheDataSource dataSource;
+  private final CacheWriter cacheWriter;
   @Nullable private final PriorityTaskManager priorityTaskManager;
 
   @Nullable private ProgressListener progressListener;
@@ -101,6 +102,15 @@ public final class ProgressiveDownloader implements Downloader {
             .setFlags(DataSpec.FLAG_ALLOW_CACHE_FRAGMENTATION)
             .build();
     dataSource = cacheDataSourceFactory.createDataSourceForDownloading();
+    @SuppressWarnings("methodref.receiver.bound.invalid")
+    CacheWriter.ProgressListener progressListener = this::onProgress;
+    cacheWriter =
+        new CacheWriter(
+            dataSource,
+            dataSpec,
+            /* allowShortContent= */ false,
+            /* temporaryBuffer= */ null,
+            progressListener);
     priorityTaskManager = cacheDataSourceFactory.getUpstreamPriorityTaskManager();
   }
 
@@ -108,28 +118,19 @@ public final class ProgressiveDownloader implements Downloader {
   public void download(@Nullable ProgressListener progressListener)
       throws IOException, InterruptedException {
     this.progressListener = progressListener;
-    if (downloadRunnable == null) {
-      CacheWriter cacheWriter =
-          new CacheWriter(
-              dataSource,
-              dataSpec,
-              /* allowShortContent= */ false,
-              /* temporaryBuffer= */ null,
-              this::onProgress);
-      downloadRunnable =
-          new RunnableFutureTask<Void, IOException>() {
-            @Override
-            protected Void doWork() throws IOException {
-              cacheWriter.cache();
-              return null;
-            }
+    downloadRunnable =
+        new RunnableFutureTask<Void, IOException>() {
+          @Override
+          protected Void doWork() throws IOException {
+            cacheWriter.cache();
+            return null;
+          }
 
-            @Override
-            protected void cancelWork() {
-              cacheWriter.cancel();
-            }
-          };
-    }
+          @Override
+          protected void cancelWork() {
+            cacheWriter.cancel();
+          }
+        };
 
     if (priorityTaskManager != null) {
       priorityTaskManager.add(C.PRIORITY_DOWNLOAD);

library/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java

@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.google.android.exoplayer2.offline;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import android.net.Uri;
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import com.google.android.exoplayer2.MediaItem;
+import com.google.android.exoplayer2.database.DatabaseProvider;
+import com.google.android.exoplayer2.testutil.FakeDataSet;
+import com.google.android.exoplayer2.testutil.FakeDataSource;
+import com.google.android.exoplayer2.testutil.TestUtil;
+import com.google.android.exoplayer2.upstream.DataSource;
+import com.google.android.exoplayer2.upstream.cache.Cache;
+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.Util;
+import java.io.File;
+import java.io.IOException;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/** Unit tests for {@link ActionFile}. */
+@SuppressWarnings("deprecation")
+@RunWith(AndroidJUnit4.class)
+public class ProgressiveDownloaderTest {
+
+  private File testDir;
+  private Cache downloadCache;
+
+  @Before
+  public void createDownloadCache() throws Exception {
+    testDir =
+        Util.createTempFile(
+            ApplicationProvider.getApplicationContext(), "ProgressiveDownloaderTest");
+    assertThat(testDir.delete()).isTrue();
+    assertThat(testDir.mkdirs()).isTrue();
+
+    DatabaseProvider databaseProvider = TestUtil.getInMemoryDatabaseProvider();
+    downloadCache = new SimpleCache(testDir, new NoOpCacheEvictor(), databaseProvider);
+  }
+
+  @After
+  public void deleteDownloadCache() {
+    downloadCache.release();
+    Util.recursiveDelete(testDir);
+  }
+
+  @Test
+  public void download_afterSingleFailure_succeeds() throws Exception {
+    Uri uri = Uri.parse("test:///test.mp4");
+
+    // Fake data has a built in failure after 10 bytes.
+    FakeDataSet data = new FakeDataSet();
+    data.newData(uri).appendReadData(10).appendReadError(new IOException()).appendReadData(20);
+    DataSource.Factory upstreamDataSource = new FakeDataSource.Factory().setFakeDataSet(data);
+
+    MediaItem mediaItem = MediaItem.fromUri(uri);
+    CacheDataSource.Factory cacheDataSourceFactory =
+        new CacheDataSource.Factory()
+            .setCache(downloadCache)
+            .setUpstreamDataSourceFactory(upstreamDataSource);
+    ProgressiveDownloader downloader = new ProgressiveDownloader(mediaItem, cacheDataSourceFactory);
+
+    TestProgressListener progressListener = new TestProgressListener();
+
+    // Failure expected after 10 bytes.
+    assertThrows(IOException.class, () -> downloader.download(progressListener));
+    assertThat(progressListener.bytesDownloaded).isEqualTo(10);
+
+    // Retry should succeed.
+    downloader.download(progressListener);
+    assertThat(progressListener.bytesDownloaded).isEqualTo(30);
+  }
+
+  private static final class TestProgressListener implements Downloader.ProgressListener {
+
+    public long bytesDownloaded;
+
+    @Override
+    public void onProgress(long contentLength, long bytesDownloaded, float percentDownloaded) {
+      this.bytesDownloaded = bytesDownloaded;
+    }
+  }
+}