Don't write CachedContentIndex to disk on key removal

Issue: #5136
PiperOrigin-RevId: 224857629

RELEASENOTES.md

@@ -12,9 +12,15 @@
     selections together.
 * Do not retry failed loads whose error is `FileNotFoundException`.
 * Prevent Cea608Decoder from generating Subtitles with null Cues list.
-* Caching: Cache data with unknown length by default. The previous flag to opt
-  in to this behavior (`DataSpec.FLAG_ALLOW_CACHING_UNKNOWN_LENGTH`) has been
-  replaced with an opt out flag (`DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN`).
+* Offline:
+  * Speed up removal of segmented downloads
+    ([#5136](https://github.com/google/ExoPlayer/issues/5136)).
+* Caching:
+  * Improve performance of `SimpleCache`.
+  * Cache data with unknown length by default. The previous flag to opt in to
+    this behavior (`DataSpec.FLAG_ALLOW_CACHING_UNKNOWN_LENGTH`) has been
+    replaced with an opt out flag
+    (`DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN`).
 * Disable post processing on Nvidia devices, as it breaks decode-only frame
   skippping.
 

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java

@@ -82,7 +82,7 @@ public interface Cache {
    * Releases the cache. This method must be called when the cache is no longer required. The cache
    * must not be used after calling this method.
    */
-  void release() throws CacheException;
+  void release();
 
   /**
    * Registers a listener to listen for changes to a given key.

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java

@@ -17,6 +17,7 @@ package com.google.android.exoplayer2.upstream.cache;
 
 import android.support.annotation.VisibleForTesting;
 import android.util.SparseArray;
+import android.util.SparseBooleanArray;
 import com.google.android.exoplayer2.upstream.cache.Cache.CacheException;
 import com.google.android.exoplayer2.util.Assertions;
 import com.google.android.exoplayer2.util.AtomicFile;
@@ -42,6 +43,7 @@ import javax.crypto.CipherOutputStream;
 import javax.crypto.NoSuchPaddingException;
 import javax.crypto.spec.IvParameterSpec;
 import javax.crypto.spec.SecretKeySpec;
+import org.checkerframework.checker.nullness.compatqual.NullableType;
 
 /** Maintains the index of cached content. */
 /* package */ class CachedContentIndex {
@@ -53,7 +55,30 @@ import javax.crypto.spec.SecretKeySpec;
   private static final int FLAG_ENCRYPTED_INDEX = 1;
 
   private final HashMap<String, CachedContent> keyToContent;
-  private final SparseArray<String> idToKey;
+  /**
+   * Maps assigned ids to their corresponding keys. Also contains (id -> null) entries for ids that
+   * have been removed from the index since it was last stored. This prevents reuse of these ids,
+   * which is necessary to avoid clashes that could otherwise occur as a result of the sequence:
+   *
+   * <p>[1] (key1, id1) is removed from the in-memory index ... the index is not stored to disk ...
+   * [2] id1 is reused for a different key2 ... the index is not stored to disk ... [3] A file for
+   * key2 is partially written using a path corresponding to id1 ... the process is killed before
+   * the index is stored to disk ... [4] The index is read from disk, causing the partially written
+   * file to be incorrectly associated to key1
+   *
+   * <p>By avoiding id reuse in step [2], a new id2 will be used instead. Step [4] will then delete
+   * the partially written file because the index does not contain an entry for id2.
+   *
+   * <p>When the index is next stored (id -> null) entries are removed, making the ids eligible for
+   * reuse.
+   */
+  private final SparseArray<@NullableType String> idToKey;
+  /**
+   * Tracks ids for which (id -> null) entries are present in idToKey, so that they can be removed
+   * efficiently when the index is next stored.
+   */
+  private final SparseBooleanArray removedIds;
+
   private final AtomicFile atomicFile;
   private final Cipher cipher;
   private final SecretKeySpec secretKeySpec;
@@ -105,6 +130,7 @@ import javax.crypto.spec.SecretKeySpec;
     }
     keyToContent = new HashMap<>();
     idToKey = new SparseArray<>();
+    removedIds = new SparseBooleanArray();
     atomicFile = new AtomicFile(new File(cacheDir, FILE_NAME));
   }
 
@@ -125,6 +151,12 @@ import javax.crypto.spec.SecretKeySpec;
     }
     writeFile();
     changed = false;
+    // Make ids that were removed since the index was last stored eligible for re-use.
+    int removedIdCount = removedIds.size();
+    for (int i = 0; i < removedIdCount; i++) {
+      idToKey.remove(removedIds.keyAt(i));
+    }
+    removedIds.clear();
   }
 
   /**
@@ -169,8 +201,11 @@ import javax.crypto.spec.SecretKeySpec;
     CachedContent cachedContent = keyToContent.get(key);
     if (cachedContent != null && cachedContent.isEmpty() && !cachedContent.isLocked()) {
       keyToContent.remove(key);
-      idToKey.remove(cachedContent.id);
       changed = true;
+      // Keep an entry in idToKey to stop the id from being reused until the index is next stored.
+      idToKey.put(cachedContent.id, /* value= */ null);
+      // Track that the entry should be removed from idToKey when the index is next stored.
+      removedIds.put(cachedContent.id, /* value= */ true);
     }
   }
 

library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java

@@ -146,13 +146,16 @@ public final class SimpleCache implements Cache {
   }
 
   @Override
-  public synchronized void release() throws CacheException {
+  public synchronized void release() {
     if (released) {
       return;
     }
     listeners.clear();
+    removeStaleSpans();
     try {
-      removeStaleSpansAndCachedContents();
+      index.store();
+    } catch (CacheException e) {
+      Log.e(TAG, "Storing index file failed", e);
     } finally {
       unlockFolder(cacheDir);
       released = true;
@@ -265,7 +268,7 @@ public final class SimpleCache implements Cache {
     if (!cacheDir.exists()) {
       // For some reason the cache directory doesn't exist. Make a best effort to create it.
       cacheDir.mkdirs();
-      removeStaleSpansAndCachedContents();
+      removeStaleSpans();
     }
     evictor.onStartFile(this, key, position, maxLength);
     return SimpleCacheSpan.getCacheFile(
@@ -311,9 +314,9 @@ public final class SimpleCache implements Cache {
   }
 
   @Override
-  public synchronized void removeSpan(CacheSpan span) throws CacheException {
+  public synchronized void removeSpan(CacheSpan span) {
     Assertions.checkState(!released);
-    removeSpan(span, true);
+    removeSpanInternal(span);
   }
 
   @Override
@@ -379,7 +382,7 @@ public final class SimpleCache implements Cache {
       if (span.isCached && !span.file.exists()) {
         // The file has been deleted from under us. It's likely that other files will have been
         // deleted too, so scan the whole in-memory representation.
-        removeStaleSpansAndCachedContents();
+        removeStaleSpans();
         continue;
       }
       return span;
@@ -431,27 +434,21 @@ public final class SimpleCache implements Cache {
     notifySpanAdded(span);
   }
 
-  private void removeSpan(CacheSpan span, boolean removeEmptyCachedContent) throws CacheException {
+  private void removeSpanInternal(CacheSpan span) {
     CachedContent cachedContent = index.get(span.key);
     if (cachedContent == null || !cachedContent.removeSpan(span)) {
       return;
     }
     totalSpace -= span.length;
-    try {
-      if (removeEmptyCachedContent) {
-        index.maybeRemove(cachedContent.key);
-        index.store();
-      }
-    } finally {
-      notifySpanRemoved(span);
-    }
+    index.maybeRemove(cachedContent.key);
+    notifySpanRemoved(span);
   }
 
   /**
    * Scans all of the cached spans in the in-memory representation, removing any for which files no
    * longer exist.
    */
-  private void removeStaleSpansAndCachedContents() throws CacheException {
+  private void removeStaleSpans() {
     ArrayList<CacheSpan> spansToBeRemoved = new ArrayList<>();
     for (CachedContent cachedContent : index.getAll()) {
       for (CacheSpan span : cachedContent.getSpans()) {
@@ -461,11 +458,8 @@ public final class SimpleCache implements Cache {
       }
     }
     for (int i = 0; i < spansToBeRemoved.size(); i++) {
-      // Remove span but not CachedContent to prevent multiple index.store() calls.
-      removeSpan(spansToBeRemoved.get(i), false);
+      removeSpanInternal(spansToBeRemoved.get(i));
     }
-    index.removeEmpty();
-    index.store();
   }
 
   private void notifySpanRemoved(CacheSpan span) {

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java

@@ -467,11 +467,7 @@ public final class CacheDataSourceTest {
     NavigableSet<CacheSpan> cachedSpans = cache.getCachedSpans(defaultCacheKey);
     for (CacheSpan cachedSpan : cachedSpans) {
       if (cachedSpan.position >= halfDataLength) {
-        try {
-          cache.removeSpan(cachedSpan);
-        } catch (Cache.CacheException e) {
-          // do nothing
-        }
+        cache.removeSpan(cachedSpan);
       }
     }
 

library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java

@@ -47,6 +47,7 @@ import org.robolectric.RuntimeEnvironment;
 public class SimpleCacheTest {
 
   private static final String KEY_1 = "key1";
+  private static final String KEY_2 = "key2";
 
   private File cacheDir;
 
@@ -152,6 +153,40 @@ public class SimpleCacheTest {
     assertCachedDataReadCorrect(cacheSpan2);
   }
 
+  @Test
+  public void testReloadCacheWithoutRelease() throws Exception {
+    SimpleCache simpleCache = getSimpleCache();
+
+    // Write data for KEY_1.
+    CacheSpan cacheSpan1 = simpleCache.startReadWrite(KEY_1, 0);
+    addCache(simpleCache, KEY_1, 0, 15);
+    simpleCache.releaseHoleSpan(cacheSpan1);
+    // Write and remove data for KEY_2.
+    CacheSpan cacheSpan2 = simpleCache.startReadWrite(KEY_2, 0);
+    addCache(simpleCache, KEY_2, 0, 15);
+    simpleCache.releaseHoleSpan(cacheSpan2);
+    simpleCache.removeSpan(simpleCache.getCachedSpans(KEY_2).first());
+
+    // Don't release the cache. This means the index file wont have been written to disk after the
+    // data for KEY_2 was removed. Move the cache instead, so we can reload it without failing the
+    // folder locking check.
+    File cacheDir2 = Util.createTempFile(RuntimeEnvironment.application, "ExoPlayerTest");
+    cacheDir2.delete();
+    cacheDir.renameTo(cacheDir2);
+
+    // Reload the cache from its new location.
+    simpleCache = new SimpleCache(cacheDir2, new NoOpCacheEvictor());
+
+    // Read data back for KEY_1.
+    CacheSpan cacheSpan3 = simpleCache.startReadWrite(KEY_1, 0);
+    assertCachedDataReadCorrect(cacheSpan3);
+
+    // Check the entry for KEY_2 was removed when the cache was reloaded.
+    assertThat(simpleCache.getCachedSpans(KEY_2)).isEmpty();
+
+    Util.recursiveDelete(cacheDir2);
+  }
+
   @Test
   public void testEncryptedIndex() throws Exception {
     byte[] key = "Bar12345Bar12345".getBytes(C.UTF8_NAME); // 128 bit key