From 0a0df933347ae7abc42027bc933bed6401892ce6 Mon Sep 17 00:00:00 2001 From: Marc Baechinger Date: Wed, 19 Oct 2022 21:10:57 +0000 Subject: [PATCH] Merge pull request #10570 from Artemych:fix/progressive_downloader_infinite_loop PiperOrigin-RevId: 472475124 (cherry picked from commit 9c56b2c4b631062731ccfc0d8e5b5c105449074c) --- .../offline/ProgressiveDownloader.java | 32 +++++++------ .../offline/ProgressiveDownloaderTest.java | 47 +++++++++++++++++++ 2 files changed, 64 insertions(+), 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 index 7af11dbbb4a..064aeff5e02 100644 --- 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() { - @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() { + @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/library/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java index 0dfe89eea3e..fca35d8a531 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/ProgressiveDownloaderTest.java +++ b/library/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;