From cccb9e1ae8805887b44b279d8afc1ca04a623e3f Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 18 May 2020 18:59:39 +0100 Subject: [PATCH] CacheUtil: Remove confusing remove(DataSpec) method - The remove(DataSpec) method was confusing because it ignored the DataSpec position and range, and instead removed all data with a matching cache key. - The remove(String) method seems better put directly on the Cache interface. PiperOrigin-RevId: 312113302 --- .../offline/ProgressiveDownloader.java | 2 +- .../exoplayer2/offline/SegmentDownloader.java | 4 +-- .../exoplayer2/upstream/cache/Cache.java | 11 ++++-- .../exoplayer2/upstream/cache/CacheUtil.java | 36 ------------------- .../cache/LeastRecentlyUsedCacheEvictor.java | 7 +--- .../upstream/cache/SimpleCache.java | 8 +++++ .../upstream/cache/CacheDataSourceTest.java | 2 +- .../upstream/cache/CacheUtilTest.java | 31 ---------------- .../upstream/cache/SimpleCacheTest.java | 16 +++++++++ 9 files changed, 38 insertions(+), 79 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 7ad3932ef8..794b537ab6 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 @@ -134,7 +134,7 @@ public final class ProgressiveDownloader implements Downloader { @Override public void remove() { - CacheUtil.remove(dataSpec, dataSource.getCache(), dataSource.getCacheKeyFactory()); + dataSource.getCache().removeSpans(dataSource.getCacheKeyFactory().buildCacheKey(dataSpec)); } private static final class ProgressForwarder implements CacheUtil.ProgressListener { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java index 544eda85d7..3269d062ee 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java @@ -205,13 +205,13 @@ public abstract class SegmentDownloader> impleme M manifest = getManifest(dataSource, manifestDataSpec); List segments = getSegments(dataSource, manifest, true); for (int i = 0; i < segments.size(); i++) { - CacheUtil.remove(segments.get(i).dataSpec, cache, cacheKeyFactory); + cache.removeSpans(cacheKeyFactory.buildCacheKey(segments.get(i).dataSpec)); } } catch (IOException e) { // Ignore exceptions when removing. } finally { // Always attempt to remove the manifest. - CacheUtil.remove(manifestDataSpec, cache, cacheKeyFactory); + cache.removeSpans(cacheKeyFactory.buildCacheKey(manifestDataSpec)); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java index 1d504159e6..b96388b8af 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java @@ -229,16 +229,23 @@ public interface Cache { */ void releaseHoleSpan(CacheSpan holeSpan); + /** + * Removes all {@link CacheSpan CacheSpans} with the given key, deleting the underlying files. + * + * @param key The cache key for the data. + */ + @WorkerThread + void removeSpans(String key); + /** * Removes a cached {@link CacheSpan} from the cache, deleting the underlying file. * *

This method may be slow and shouldn't normally be called on the main thread. * * @param span The {@link CacheSpan} to remove. - * @throws CacheException If an error is encountered. */ @WorkerThread - void removeSpan(CacheSpan span) throws CacheException; + void removeSpan(CacheSpan span); /** * Queries if a range is entirely available in the cache. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java index 08622df3ea..47b61780d8 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java @@ -28,7 +28,6 @@ import com.google.android.exoplayer2.util.Util; import java.io.EOFException; import java.io.IOException; import java.io.InterruptedIOException; -import java.util.NavigableSet; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -314,41 +313,6 @@ public final class CacheUtil { } } - /** - * Removes all of the data specified by the {@code dataSpec}. - * - *

This methods blocks until the operation is complete. - * - * @param dataSpec Defines the data to be removed. - * @param cache A {@link Cache} to store the data. - * @param cacheKeyFactory An optional factory for cache keys. - */ - @WorkerThread - public static void remove( - DataSpec dataSpec, Cache cache, @Nullable CacheKeyFactory cacheKeyFactory) { - remove(cache, buildCacheKey(dataSpec, cacheKeyFactory)); - } - - /** - * Removes all of the data specified by the {@code key}. - * - *

This methods blocks until the operation is complete. - * - * @param cache A {@link Cache} to store the data. - * @param key The key whose data should be removed. - */ - @WorkerThread - public static void remove(Cache cache, String key) { - NavigableSet cachedSpans = cache.getCachedSpans(key); - for (CacheSpan cachedSpan : cachedSpans) { - try { - cache.removeSpan(cachedSpan); - } catch (Cache.CacheException e) { - // Do nothing. - } - } - } - private static String buildCacheKey( DataSpec dataSpec, @Nullable CacheKeyFactory cacheKeyFactory) { return (cacheKeyFactory != null ? cacheKeyFactory : CacheKeyFactory.DEFAULT) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java index c88e2643d8..fb461813ae 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java @@ -16,7 +16,6 @@ package com.google.android.exoplayer2.upstream.cache; import com.google.android.exoplayer2.C; -import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import java.util.TreeSet; /** Evicts least recently used cache files first. */ @@ -70,11 +69,7 @@ public final class LeastRecentlyUsedCacheEvictor implements CacheEvictor { private void evictCache(Cache cache, long requiredSpace) { while (currentSize + requiredSpace > maxBytes && !leastRecentlyUsed.isEmpty()) { - try { - cache.removeSpan(leastRecentlyUsed.first()); - } catch (CacheException e) { - // do nothing. - } + cache.removeSpan(leastRecentlyUsed.first()); } } 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 721dac4d4e..5d3f430d6e 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 @@ -470,6 +470,14 @@ public final class SimpleCache implements Cache { notifyAll(); } + @Override + public synchronized void removeSpans(String key) { + Assertions.checkState(!released); + for (CacheSpan span : getCachedSpans(key)) { + removeSpanInternal(span); + } + } + @Override public synchronized void removeSpan(CacheSpan span) { Assertions.checkState(!released); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java index f6ba0a1e63..133c6b3d73 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java @@ -432,7 +432,7 @@ public final class CacheDataSourceTest { TestUtil.readExactly(cacheDataSource, 100); // Delete cached data. - CacheUtil.remove(unboundedDataSpec, cache, /* cacheKeyFactory= */ null); + cache.removeSpans(cacheDataSource.getCacheKeyFactory().buildCacheKey(unboundedDataSpec)); assertCacheEmpty(cache); // Read the rest of the data. diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java index 9acc9b11c8..c4115cbc28 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java @@ -15,7 +15,6 @@ */ package com.google.android.exoplayer2.upstream.cache; -import static com.google.android.exoplayer2.testutil.CacheAsserts.assertCacheEmpty; import static com.google.android.exoplayer2.testutil.CacheAsserts.assertCachedData; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; @@ -29,7 +28,6 @@ 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.DataSpec; -import com.google.android.exoplayer2.upstream.FileDataSource; import com.google.android.exoplayer2.util.Util; import java.io.EOFException; import java.io.File; @@ -287,35 +285,6 @@ public final class CacheUtilTest { assertCachedData(cache, fakeDataSet); } - @Test - public void remove() throws Exception { - FakeDataSet fakeDataSet = new FakeDataSet().setRandomData("test_data", 100); - FakeDataSource dataSource = new FakeDataSource(fakeDataSet); - - DataSpec dataSpec = - new DataSpec.Builder() - .setUri("test_data") - .setFlags(DataSpec.FLAG_ALLOW_CACHE_FRAGMENTATION) - .build(); - CacheUtil.cache( - // Set fragmentSize to 10 to make sure there are multiple spans. - new CacheDataSource( - cache, - dataSource, - new FileDataSource(), - new CacheDataSink(cache, /* fragmentSize= */ 10), - /* flags= */ 0, - /* eventListener= */ null), - dataSpec, - /* progressListener= */ null, - /* isCanceled= */ null, - /* enableEOFException= */ true, - /* temporaryBuffer= */ new byte[CacheUtil.DEFAULT_BUFFER_SIZE_BYTES]); - CacheUtil.remove(dataSpec, cache, /* cacheKeyFactory= */ null); - - assertCacheEmpty(cache); - } - private static final class CachingCounters implements CacheUtil.ProgressListener { private long contentLength = C.LENGTH_UNSET; 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 14222f144d..08c63443b4 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 @@ -225,6 +225,22 @@ public class SimpleCacheTest { Util.recursiveDelete(cacheDir2); } + @Test + public void removeSpans_removesSpansWithSameKey() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + CacheSpan holeSpan = simpleCache.startReadWrite(KEY_1, 0); + addCache(simpleCache, KEY_1, 0, 10); + addCache(simpleCache, KEY_1, 20, 10); + simpleCache.releaseHoleSpan(holeSpan); + holeSpan = simpleCache.startReadWrite(KEY_2, 20); + addCache(simpleCache, KEY_2, 20, 10); + simpleCache.releaseHoleSpan(holeSpan); + + simpleCache.removeSpans(KEY_1); + assertThat(simpleCache.getCachedSpans(KEY_1)).isEmpty(); + assertThat(simpleCache.getCachedSpans(KEY_2)).hasSize(1); + } + @Test public void encryptedIndex() throws Exception { byte[] key = Util.getUtf8Bytes("Bar12345Bar12345"); // 128 bit key