From 09248608c08ebd349b5805a9e4685b147c41e55f Mon Sep 17 00:00:00 2001 From: eguven Date: Tue, 19 Sep 2017 09:10:35 -0700 Subject: [PATCH] Notify span listeners even if index store fails in SimpleCache.removeSpan This fixes infinite loop in LeastRecentlyUsedCacheEvictor.evictCache when index store fails. Also made CachedContentIndex not final so it can be mocked and added a package protected SimpleCache constructor so mock index can be injected. Issue: #3260 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=169249517 --- .../upstream/cache/CachedContentIndex.java | 2 +- .../upstream/cache/SimpleCache.java | 27 ++++++++--- .../upstream/cache/SimpleCacheTest.java | 45 ++++++++++++++++++- 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java index f043771e30..5d92c51dcc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java @@ -49,7 +49,7 @@ import javax.crypto.spec.SecretKeySpec; /** * This class maintains the index of cached content. */ -/*package*/ final class CachedContentIndex { +/*package*/ class CachedContentIndex { public static final String FILE_NAME = "cached_content_index.exi"; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java index bb1ac83698..2fe16287db 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java @@ -47,7 +47,7 @@ public final class SimpleCache implements Cache { * @param evictor The evictor to be used. */ public SimpleCache(File cacheDir, CacheEvictor evictor) { - this(cacheDir, evictor, null); + this(cacheDir, evictor, null, false); } /** @@ -74,10 +74,22 @@ public final class SimpleCache implements Cache { * @param encrypt When false, a plaintext index will be written. */ public SimpleCache(File cacheDir, CacheEvictor evictor, byte[] secretKey, boolean encrypt) { + this(cacheDir, evictor, new CachedContentIndex(cacheDir, secretKey, encrypt)); + } + + /** + * Constructs the cache. The cache will delete any unrecognized files from the directory. Hence + * the directory cannot be used to store other files. + * + * @param cacheDir A dedicated cache directory. + * @param evictor The evictor to be used. + * @param index The CachedContentIndex to be used. + */ + /*package*/ SimpleCache(File cacheDir, CacheEvictor evictor, CachedContentIndex index) { this.cacheDir = cacheDir; this.evictor = evictor; this.lockedSpans = new HashMap<>(); - this.index = new CachedContentIndex(cacheDir, secretKey, encrypt); + this.index = index; this.listeners = new HashMap<>(); // Start cache initialization. final ConditionVariable conditionVariable = new ConditionVariable(); @@ -304,11 +316,14 @@ public final class SimpleCache implements Cache { return; } totalSpace -= span.length; - if (removeEmptyCachedContent && cachedContent.isEmpty()) { - index.removeEmpty(cachedContent.key); - index.store(); + try { + if (removeEmptyCachedContent && cachedContent.isEmpty()) { + index.removeEmpty(cachedContent.key); + index.store(); + } + } finally { + notifySpanRemoved(span); } - notifySpanRemoved(span); } @Override diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java index ed55045835..d5894895b1 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java @@ -18,8 +18,10 @@ package com.google.android.exoplayer2.upstream.cache; import static com.google.android.exoplayer2.C.LENGTH_UNSET; import static com.google.android.exoplayer2.util.Util.toByteArray; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.doAnswer; import com.google.android.exoplayer2.C; +import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import com.google.android.exoplayer2.util.Util; import java.io.File; import java.io.FileInputStream; @@ -29,9 +31,14 @@ import java.util.NavigableSet; import java.util.Random; import java.util.Set; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.robolectric.RobolectricTestRunner; import org.robolectric.RuntimeEnvironment; import org.robolectric.annotation.Config; @@ -49,6 +56,7 @@ public class SimpleCacheTest { @Before public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); cacheDir = Util.createTempDirectory(RuntimeEnvironment.application, "ExoPlayerTest"); } @@ -209,7 +217,6 @@ public class SimpleCacheTest { assertThat(cacheDir.listFiles()).hasLength(0); } - @Test public void testGetCachedBytes() throws Exception { SimpleCache simpleCache = getSimpleCache(); @@ -245,6 +252,42 @@ public class SimpleCacheTest { simpleCache.releaseHoleSpan(cacheSpan); } + /* Tests https://github.com/google/ExoPlayer/issues/3260 case. */ + @Test + public void testExceptionDuringEvictionByLeastRecentlyUsedCacheEvictorNotHang() throws Exception { + CachedContentIndex index = Mockito.spy(new CachedContentIndex(cacheDir)); + SimpleCache simpleCache = + new SimpleCache(cacheDir, new LeastRecentlyUsedCacheEvictor(20), index); + + // Add some content. + CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, 0); + addCache(simpleCache, KEY_1, 0, 15); + + // Make index.store() throw exception from now on. + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + throw new Cache.CacheException("SimpleCacheTest"); + } + }).when(index).store(); + + // Adding more content will make LeastRecentlyUsedCacheEvictor evict previous content. + try { + addCache(simpleCache, KEY_1, 15, 15); + Assert.fail("Exception was expected"); + } catch (CacheException e) { + // do nothing. + } + + simpleCache.releaseHoleSpan(cacheSpan); + + // Although store() has failed, it should remove the first span and add the new one. + NavigableSet cachedSpans = simpleCache.getCachedSpans(KEY_1); + assertThat(cachedSpans).isNotNull(); + assertThat(cachedSpans).hasSize(1); + assertThat(cachedSpans.pollFirst().position).isEqualTo(15); + } + private SimpleCache getSimpleCache() { return new SimpleCache(cacheDir, new NoOpCacheEvictor()); }