From 37d9e2f485f9c517d83ee9a71caa6ed9b8549550 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 28 Apr 2020 15:28:25 +0100 Subject: [PATCH] DownloadManagerTest: Improve thread interactions - Remove assertReleased and replace it with a proper condition variable that's opened when Downloader.download or Download.remove finish. As far as I can tell assertReleased was basically implementing "sleep for 10 seconds after the Downloader starts". Note fixing this properly also makes the tests run much faster! - Use ConditionVariable instead of CountDownLatch(1). - Use AtomicInteger instead of volatile int because it's clearer and allows removal of explanatory comments. PiperOrigin-RevId: 308819204 --- .../offline/DownloadManagerTest.java | 223 +++++++++--------- .../dash/offline/DownloadManagerDashTest.java | 3 +- .../testutil/TestDownloadManagerListener.java | 40 +--- 3 files changed, 126 insertions(+), 140 deletions(-) diff --git a/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java index 080bf2f4fa..69c494e23c 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java @@ -28,14 +28,14 @@ import com.google.android.exoplayer2.testutil.DummyMainThread; import com.google.android.exoplayer2.testutil.DummyMainThread.TestRunnable; import com.google.android.exoplayer2.testutil.TestDownloadManagerListener; import com.google.android.exoplayer2.testutil.TestUtil; +import com.google.android.exoplayer2.util.ConditionVariable; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -54,10 +54,6 @@ public class DownloadManagerTest { private static final int ASSERT_TRUE_TIMEOUT = 10000; /** Used to check if condition stays false for this time interval. */ private static final int ASSERT_FALSE_TIME = 1000; - /** Maximum retry delay in DownloadManager. */ - private static final int MAX_RETRY_DELAY = 5000; - /** Maximum number of times a downloader can be restarted before doing a released check. */ - private static final int MAX_STARTS_BEFORE_RELEASED = 1; /** A stop reason. */ private static final int APP_STOP_REASON = 1; /** The minimum number of times a task must be retried before failing. */ @@ -110,7 +106,7 @@ public class DownloadManagerTest { public void postDownloadRequest_downloads() throws Throwable { DownloadRunner runner = new DownloadRunner(uri1).postDownloadRequest(); runner.assertDownloading(); - runner.getDownloader(0).unblock().assertReleased().assertStartCount(1); + runner.getDownloader(0).unblock().assertCompleted().assertStartCount(1); runner.assertCompleted(); runner.assertCreatedDownloaderCount(1); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); @@ -121,7 +117,7 @@ public class DownloadManagerTest { public void postRemoveRequest_removes() throws Throwable { DownloadRunner runner = new DownloadRunner(uri1).postDownloadRequest().postRemoveRequest(); runner.assertRemoving(); - runner.getDownloader(1).unblock().assertReleased().assertStartCount(1); + runner.getDownloader(1).unblock().assertCompleted().assertStartCount(1); runner.assertRemoved(); runner.assertCreatedDownloaderCount(2); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); @@ -135,10 +131,10 @@ public class DownloadManagerTest { FakeDownloader downloader = runner.getDownloader(0); for (int i = 0; i <= MIN_RETRY_COUNT; i++) { - downloader.assertStarted(MAX_RETRY_DELAY).fail(); + downloader.assertStarted().fail(); } - downloader.assertReleased().assertStartCount(MIN_RETRY_COUNT + 1); + downloader.assertCompleted().assertStartCount(MIN_RETRY_COUNT + 1); runner.assertFailed(); downloadManagerListener.blockUntilTasksComplete(); assertThat(downloadManager.getCurrentDownloads()).isEmpty(); @@ -151,11 +147,11 @@ public class DownloadManagerTest { FakeDownloader downloader = runner.getDownloader(0); for (int i = 0; i < MIN_RETRY_COUNT; i++) { - downloader.assertStarted(MAX_RETRY_DELAY).fail(); + downloader.assertStarted().fail(); } - downloader.assertStarted(MAX_RETRY_DELAY).unblock(); + downloader.assertStarted().unblock(); - downloader.assertReleased().assertStartCount(MIN_RETRY_COUNT + 1); + downloader.assertCompleted().assertStartCount(MIN_RETRY_COUNT + 1); runner.assertCompleted(); downloadManagerListener.blockUntilTasksComplete(); assertThat(downloadManager.getCurrentDownloads()).isEmpty(); @@ -170,11 +166,11 @@ public class DownloadManagerTest { int tooManyRetries = MIN_RETRY_COUNT + 10; for (int i = 0; i < tooManyRetries; i++) { downloader.incrementBytesDownloaded(); - downloader.assertStarted(MAX_RETRY_DELAY).fail(); + downloader.assertStarted().fail(); } - downloader.assertStarted(MAX_RETRY_DELAY).unblock(); + downloader.assertStarted().unblock(); - downloader.assertReleased().assertStartCount(tooManyRetries + 1); + downloader.assertCompleted().assertStartCount(tooManyRetries + 1); runner.assertCompleted(); downloadManagerListener.blockUntilTasksComplete(); } @@ -189,7 +185,7 @@ public class DownloadManagerTest { runner.postRemoveRequest(); downloader1.assertCanceled().assertStartCount(1); - runner.getDownloader(1).unblock().assertNotCanceled(); + runner.getDownloader(1).unblock().assertCompleted(); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); } @@ -202,8 +198,8 @@ public class DownloadManagerTest { downloader1.assertStarted(); runner.postDownloadRequest(); - downloader1.unblock().assertNotCanceled(); - runner.getDownloader(2).unblock().assertNotCanceled(); + downloader1.unblock().assertCompleted(); + runner.getDownloader(2).unblock().assertCompleted(); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); } @@ -216,7 +212,7 @@ public class DownloadManagerTest { downloader1.assertStarted(); runner.postRemoveRequest(); - downloader1.unblock().assertNotCanceled(); + downloader1.unblock().assertCompleted(); runner.assertRemoved(); runner.assertCreatedDownloaderCount(2); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); @@ -378,7 +374,7 @@ public class DownloadManagerTest { runner1.assertQueued(); // remove requests aren't stopped. - runner2.getDownloader(1).unblock().assertReleased(); + runner2.getDownloader(1).unblock().assertCompleted(); runner2.assertQueued(); // Although remove2 is finished, download2 doesn't start. runner2.getDownloader(2).assertDoesNotStart(); @@ -447,7 +443,7 @@ public class DownloadManagerTest { runner1.assertStopped(); // Other downloads aren't affected. - runner2.getDownloader(1).unblock().assertReleased(); + runner2.getDownloader(1).unblock().assertCompleted(); // New download requests can be added and they start. runner3.postDownloadRequest().getDownloader(0).assertStarted().unblock(); @@ -540,7 +536,7 @@ public class DownloadManagerTest { downloadManagerListener = new TestDownloadManagerListener(downloadManager, dummyMainThread); }); - downloadManagerListener.waitUntilInitialized(); + downloadManagerListener.blockUntilInitialized(); } catch (Throwable throwable) { throw new Exception(throwable); } @@ -696,51 +692,111 @@ public class DownloadManagerTest { private static final class FakeDownloader implements Downloader { - private final com.google.android.exoplayer2.util.ConditionVariable blocker; - private DownloadRequest request; - private CountDownLatch started; + + private final ConditionVariable started; + private final ConditionVariable finished; + private final ConditionVariable blocker; + private final AtomicInteger startCount; + private final AtomicInteger bytesDownloaded; + private volatile boolean interrupted; - private volatile boolean cancelled; + private volatile boolean canceled; private volatile boolean enableDownloadIOException; - private volatile int startCount; - private volatile int bytesDownloaded; private FakeDownloader() { - this.started = new CountDownLatch(1); - this.blocker = new com.google.android.exoplayer2.util.ConditionVariable(); - } - - @SuppressWarnings({"NonAtomicOperationOnVolatileField", "NonAtomicVolatileUpdate"}) - @Override - public void download(ProgressListener listener) throws InterruptedException, IOException { - // It's ok to update this directly as no other thread will update it. - startCount++; - started.countDown(); - block(); - if (bytesDownloaded > 0) { - listener.onProgress(C.LENGTH_UNSET, bytesDownloaded, C.PERCENTAGE_UNSET); - } - if (enableDownloadIOException) { - enableDownloadIOException = false; - throw new IOException(); - } + started = TestUtil.createRobolectricConditionVariable(); + finished = TestUtil.createRobolectricConditionVariable(); + blocker = TestUtil.createRobolectricConditionVariable(); + startCount = new AtomicInteger(); + bytesDownloaded = new AtomicInteger(); } @Override public void cancel() { - cancelled = true; + canceled = true; + } + + @Override + public void download(ProgressListener listener) throws InterruptedException, IOException { + startCount.incrementAndGet(); + started.open(); + try { + block(); + int bytesDownloaded = this.bytesDownloaded.get(); + if (listener != null && bytesDownloaded > 0) { + listener.onProgress(C.LENGTH_UNSET, bytesDownloaded, C.PERCENTAGE_UNSET); + } + if (enableDownloadIOException) { + enableDownloadIOException = false; + throw new IOException(); + } + } finally { + finished.open(); + } } - @SuppressWarnings({"NonAtomicOperationOnVolatileField", "NonAtomicVolatileUpdate"}) @Override public void remove() throws InterruptedException { - // It's ok to update this directly as no other thread will update it. - startCount++; - started.countDown(); - block(); + startCount.incrementAndGet(); + started.open(); + try { + block(); + } finally { + finished.open(); + } } + /** Unblocks {@link #download} or {@link #remove}, allowing the task to finish successfully. */ + public FakeDownloader unblock() { + blocker.open(); + return this; + } + + /** Fails {@link #download} or {@link #remove}, allowing the task to finish with an error. */ + public FakeDownloader fail() { + enableDownloadIOException = true; + blocker.open(); + return this; + } + + /** Increments the number of bytes that the fake downloader has downloaded. */ + public void incrementBytesDownloaded() { + bytesDownloaded.incrementAndGet(); + } + + public FakeDownloader assertStarted() throws InterruptedException { + assertThat(started.block(ASSERT_TRUE_TIMEOUT)).isTrue(); + started.close(); + return this; + } + + public FakeDownloader assertStartCount(int count) { + assertThat(startCount.get()).isEqualTo(count); + return this; + } + + public FakeDownloader assertCompleted() throws InterruptedException { + blockUntilFinished(); + assertThat(interrupted).isFalse(); + assertThat(canceled).isFalse(); + return this; + } + + public FakeDownloader assertCanceled() throws InterruptedException { + blockUntilFinished(); + assertThat(interrupted).isTrue(); + assertThat(canceled).isTrue(); + return this; + } + + public void assertDoesNotStart() throws InterruptedException { + Thread.sleep(ASSERT_FALSE_TIME); + assertThat(started.isOpen()).isFalse(); + } + + // Internal methods. + private void block() throws InterruptedException { try { blocker.block(); @@ -752,64 +808,9 @@ public class DownloadManagerTest { } } - private FakeDownloader assertStarted() throws InterruptedException { - return assertStarted(ASSERT_TRUE_TIMEOUT); - } - - private FakeDownloader assertStarted(int timeout) throws InterruptedException { - assertThat(started.await(timeout, TimeUnit.MILLISECONDS)).isTrue(); - started = new CountDownLatch(1); - return this; - } - - private FakeDownloader assertStartCount(int count) { - assertThat(startCount).isEqualTo(count); - return this; - } - - private FakeDownloader assertReleased() throws InterruptedException { - int count = 0; - while (started.await(ASSERT_TRUE_TIMEOUT, TimeUnit.MILLISECONDS)) { - if (count++ >= MAX_STARTS_BEFORE_RELEASED) { - fail(); - } - started = new CountDownLatch(1); - } - return this; - } - - private FakeDownloader assertCanceled() throws InterruptedException { - assertReleased(); - assertThat(interrupted).isTrue(); - assertThat(cancelled).isTrue(); - return this; - } - - private FakeDownloader assertNotCanceled() throws InterruptedException { - assertReleased(); - assertThat(interrupted).isFalse(); - assertThat(cancelled).isFalse(); - return this; - } - - private FakeDownloader unblock() { - blocker.open(); - return this; - } - - private FakeDownloader fail() { - enableDownloadIOException = true; - return unblock(); - } - - private void assertDoesNotStart() throws InterruptedException { - Thread.sleep(ASSERT_FALSE_TIME); - assertThat(started.getCount()).isEqualTo(1); - } - - @SuppressWarnings({"NonAtomicOperationOnVolatileField", "NonAtomicVolatileUpdate"}) - private void incrementBytesDownloaded() { - bytesDownloaded++; + private void blockUntilFinished() throws InterruptedException { + assertThat(finished.block(ASSERT_TRUE_TIMEOUT)).isTrue(); + finished.close(); } } } diff --git a/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadManagerDashTest.java b/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadManagerDashTest.java index 164299fb06..616a743865 100644 --- a/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadManagerDashTest.java +++ b/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadManagerDashTest.java @@ -264,8 +264,7 @@ public class DownloadManagerDashTest { downloadManager.setRequirements(new Requirements(0)); downloadManagerListener = - new TestDownloadManagerListener( - downloadManager, dummyMainThread, /* timeoutMs= */ 3000); + new TestDownloadManagerListener(downloadManager, dummyMainThread); downloadManager.resumeDownloads(); }); } diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java index 48e85427b1..1d5e9dc779 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java @@ -22,41 +22,32 @@ import androidx.annotation.Nullable; import com.google.android.exoplayer2.offline.Download; import com.google.android.exoplayer2.offline.Download.State; import com.google.android.exoplayer2.offline.DownloadManager; -import com.google.android.exoplayer2.util.Util; +import com.google.android.exoplayer2.util.ConditionVariable; import java.util.HashMap; import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** A {@link DownloadManager.Listener} for testing. */ public final class TestDownloadManagerListener implements DownloadManager.Listener { - private static final int TIMEOUT_MS = 1000; - private static final int INITIALIZATION_TIMEOUT_MS = 10_000; + private static final int TIMEOUT_MS = 10_000; private static final int STATE_REMOVED = -1; private final DownloadManager downloadManager; private final DummyMainThread dummyMainThread; private final HashMap> downloadStates; - private final CountDownLatch initializedCondition; - private final int timeoutMs; + private final ConditionVariable initializedCondition; + private final ConditionVariable idleCondition; - private @MonotonicNonNull CountDownLatch downloadFinishedCondition; @Download.FailureReason private int failureReason; public TestDownloadManagerListener( DownloadManager downloadManager, DummyMainThread dummyMainThread) { - this(downloadManager, dummyMainThread, TIMEOUT_MS); - } - - public TestDownloadManagerListener( - DownloadManager downloadManager, DummyMainThread dummyMainThread, int timeoutMs) { this.downloadManager = downloadManager; this.dummyMainThread = dummyMainThread; - this.timeoutMs = timeoutMs; downloadStates = new HashMap<>(); - initializedCondition = new CountDownLatch(1); + initializedCondition = TestUtil.createRobolectricConditionVariable(); + idleCondition = TestUtil.createRobolectricConditionVariable(); downloadManager.addListener(this); } @@ -67,13 +58,12 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen @Override public void onInitialized(DownloadManager downloadManager) { - initializedCondition.countDown(); + initializedCondition.open(); } - public void waitUntilInitialized() throws InterruptedException { + public void blockUntilInitialized() throws InterruptedException { if (!downloadManager.isInitialized()) { - assertThat(initializedCondition.await(INITIALIZATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)) - .isTrue(); + assertThat(initializedCondition.block(TIMEOUT_MS)).isTrue(); } } @@ -92,9 +82,7 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen @Override public synchronized void onIdle(DownloadManager downloadManager) { - if (downloadFinishedCondition != null) { - downloadFinishedCondition.countDown(); - } + idleCondition.open(); } /** @@ -110,16 +98,14 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen /** Blocks until all remove and download tasks are complete. Task errors are ignored. */ public void blockUntilTasksComplete() throws InterruptedException { - synchronized (this) { - downloadFinishedCondition = new CountDownLatch(1); - } + idleCondition.close(); dummyMainThread.runOnMainThread( () -> { if (downloadManager.isIdle()) { - Util.castNonNull(downloadFinishedCondition).countDown(); + idleCondition.open(); } }); - assertThat(downloadFinishedCondition.await(timeoutMs, TimeUnit.MILLISECONDS)).isTrue(); + assertThat(idleCondition.block(TIMEOUT_MS)).isTrue(); } private ArrayBlockingQueue getStateQueue(String taskId) {