Skip to content

Commit

Permalink
Reset DiskLruCachWrapper outside of IOException catch block in clear().
Browse files Browse the repository at this point in the history
DiskLruCache can close the disk cache but throw anyway in delete(), 
which previously left the disk cache in an unrecoverable state until the
application was restarted. By always nulling out the disk cache variable
we allow for future attempts at opening the disk cache even if closing
it throws. 

This particular case was caused by a bug in DiskLruCache where it would
throw on an empty directory if the cache were deleted outside the app
while the app was still running, but broadly speaking it seems like a 
bad idea to allow our disk cache to get into a state where the app must
be killed for it to function again, even if we might partially ignore
a clear disk cache command (which wouldn’t be retried or recovered in 
the previous implementation anyway).

Fixes #2465.
  • Loading branch information
sjudd committed Nov 27, 2017
1 parent fa4100f commit 16eaa9b
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 15 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
package com.bumptech.glide.test;

import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.mock;

import android.content.Context;
import android.graphics.drawable.Drawable;
import android.support.test.InstrumentationRegistry;
import android.support.test.runner.AndroidJUnit4;
import com.bumptech.glide.Glide;
import com.bumptech.glide.GlideBuilder;
import com.bumptech.glide.load.Key;
import com.bumptech.glide.load.engine.cache.DiskCache;
import com.bumptech.glide.load.engine.cache.DiskCache.Factory;
import com.bumptech.glide.load.engine.cache.DiskLruCacheWrapper;
import com.bumptech.glide.test.ResourceIds.raw;
import java.io.File;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

// Tests #2465.
@RunWith(AndroidJUnit4.class)
public class ExternallyClearedDiskCacheTest {
@Rule public final TearDownGlide tearDownGlide = new TearDownGlide();
private final ConcurrencyHelper concurrency = new ConcurrencyHelper();
private Context context;
private File cacheDir;

@Before
public void setUp() {
context = InstrumentationRegistry.getTargetContext();
cacheDir = context.getCacheDir();
}

@After
public void tearDown() {
// Force us to wait until Glide's threads shut down.
Glide.tearDown();
deleteRecursively(cacheDir);
}

@Test
public void clearDiskCache_afterOpeningDiskCache_andDeleteDirectoryOutsideGlide_doesNotThrow() {
DiskCache cache = DiskLruCacheWrapper.create(cacheDir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursively(cacheDir);
cache.clear();
}

@Test
public void get_afterDeleteDirectoryOutsideGlideAndClose_doesNotThrow() {
DiskCache cache = DiskLruCacheWrapper.create(cacheDir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursively(cacheDir);
cache.clear();

cache.get(mock(Key.class));
}

@Test
public void loadFromCache_afterDiskCacheDeletedAndCleared_doesNotFail() {
final DiskCache cache = DiskLruCacheWrapper.create(cacheDir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursively(cacheDir);
cache.clear();

Glide.init(
context,
new GlideBuilder()
.setDiskCache(new Factory() {
@Override
public DiskCache build() {
return cache;
}
}));

Drawable drawable =
concurrency.get(
Glide.with(context)
.load(ResourceIds.raw.canonical)
.submit());
assertThat(drawable).isNotNull();
}

@Test
public void loadFromCache_afterDiskCacheDeleted_doesNotFail() {
final DiskCache cache = DiskLruCacheWrapper.create(cacheDir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursively(cacheDir);

Glide.init(
context,
new GlideBuilder()
.setDiskCache(new Factory() {
@Override
public DiskCache build() {
return cache;
}
}));

Drawable drawable =
concurrency.get(Glide.with(context)
.load(raw.canonical)
.submit());
assertThat(drawable).isNotNull();
}

private static void deleteRecursively(File file) {
if (file.isDirectory()) {
File[] files = file.listFiles();
if (files != null) {
for (File f : files) {
deleteRecursively(f);
}
}
}
if (!file.delete() && file.exists()) {
throw new RuntimeException("Failed to delete: " + file);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,15 @@ public void delete(Key key) {
public synchronized void clear() {
try {
getDiskCache().delete();
resetDiskCache();
} catch (IOException e) {
if (Log.isLoggable(TAG, Log.WARN)) {
Log.w(TAG, "Unable to clear disk cache", e);
Log.w(TAG, "Unable to clear disk cache or disk cache cleared externally", e);
}
} finally {
// Delete can close the cache but still throw. If we don't null out the disk cache here, every
// subsequent request will try to act on a closed disk cache and fail. By nulling out the disk
// cache we at least allow for attempts to open the cache in the future. See #2465.
resetDiskCache();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;

import com.bumptech.glide.load.Key;
import com.bumptech.glide.signature.ObjectKey;
import com.bumptech.glide.tests.Util;
import java.io.File;
Expand Down Expand Up @@ -41,21 +43,17 @@ public void tearDown() {
}
}

private void deleteRecursive(File file) {
if (!file.isDirectory()) {
if (!file.delete()) {
throw new IllegalStateException("Failed to delete: " + file);
private static void deleteRecursive(File file) {
if (file.isDirectory()) {
File[] files = file.listFiles();
if (files != null) {
for (File f : files) {
deleteRecursive(f);
}
}
return;
}

File[] files = file.listFiles();
if (files == null) {
return;
}

for (File child : files) {
deleteRecursive(child);
if (!file.delete() && file.exists()) {
throw new RuntimeException("Failed to delete: " + file);
}
}

Expand Down Expand Up @@ -136,4 +134,24 @@ public boolean write(File file) {

assertArrayEquals(data, received);
}

// Tests #2465.
@Test
public void clearDiskCache_afterOpeningDiskCache_andDeleteDirectoryOutsideGlide_doesNotThrow() {
DiskCache cache = DiskLruCacheWrapper.create(dir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursive(dir);
cache.clear();
}

// Tests #2465.
@Test
public void get_afterDeleteDirectoryOutsideGlideAndClose_doesNotThrow() {
DiskCache cache = DiskLruCacheWrapper.create(dir, 1024 * 1024);
cache.get(mock(Key.class));
deleteRecursive(dir);
cache.clear();

cache.get(mock(Key.class));
}
}

0 comments on commit 16eaa9b

Please sign in to comment.