Skip to content

Commit

Permalink
Enforce disk size limit more strictly in DiskBasedCache.
Browse files Browse the repository at this point in the history 
-Previously, when new entries were written to the cache with put(),
the size accounted for in those entries did not include cache overhead
when actually storing them on disk. That overhead is now included.

-We now prune excess entries after writing new entries to disk instead
of before (so that we know the size-on-disk). This prevents the cache
from exceeding the size limit due to one large entry (until the next
put). The cache will now exceed the maximum size when writing an entry
which pushes it over the limit; however, this excess will be cleared
before put() returns rather than some indefinite time in the future.

Fixes google#57
  • Loading branch information
@jpd236
jpd236 committed May 31, 2018
1 parent 0c32d6a commit 1476fac
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 37 deletions.
38 changes: 19 additions & 19 deletions src/main/java/com/android/volley/toolbox/DiskBasedCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public class DiskBasedCache implements Cache {
private static final int DEFAULT_DISK_USAGE_BYTES = 5 * 1024 * 1024;

/** High water mark percentage for the cache */
private static final float HYSTERESIS_FACTOR = 0.9f;
@VisibleForTesting static final float HYSTERESIS_FACTOR = 0.9f;

/** Magic number for current version of cache file format. */
private static final int CACHE_MAGIC = 0x20150306;
Expand All @@ -74,7 +74,9 @@ public class DiskBasedCache implements Cache {
* Constructs an instance of the DiskBasedCache at the specified directory.
*
* @param rootDirectory The root directory of the cache.
* @param maxCacheSizeInBytes The maximum size of the cache in bytes.
* @param maxCacheSizeInBytes The maximum size of the cache in bytes. Note that the cache may
* briefly exceed this size on disk when writing a new entry that pushes it over the limit
* until the ensuing pruning completes.
*/
public DiskBasedCache(File rootDirectory, int maxCacheSizeInBytes) {
mRootDirectory = rootDirectory;
Expand Down Expand Up @@ -166,8 +168,6 @@ public synchronized void initialize() {
new BufferedInputStream(createInputStream(file)), entrySize);
try {
CacheHeader entry = CacheHeader.readHeader(cis);
// NOTE: When this entry was put, its size was recorded as data.length, but
// when the entry is initialized below, its size is recorded as file.length()
entry.size = entrySize;
putEntry(entry.key, entry);
} finally {
Expand Down Expand Up @@ -203,7 +203,6 @@ public synchronized void invalidate(String key, boolean fullExpire) {
/** Puts the entry with the specified key into the cache. */
@Override
public synchronized void put(String key, Entry entry) {
pruneIfNeeded(entry.data.length);
File file = getFileForKey(key);
try {
BufferedOutputStream fos = new BufferedOutputStream(createOutputStream(file));
Expand All @@ -216,7 +215,9 @@ public synchronized void put(String key, Entry entry) {
}
fos.write(entry.data);
fos.close();
e.size = file.length();
putEntry(key, e);
pruneIfNeeded();
return;
} catch (IOException e) {
}
Expand Down Expand Up @@ -256,13 +257,9 @@ public File getFileForKey(String key) {
return new File(mRootDirectory, getFilenameForKey(key));
}

/**
* Prunes the cache to fit the amount of bytes specified.
*
* @param neededSpace The amount of bytes we are trying to fit into the cache.
*/
private void pruneIfNeeded(int neededSpace) {
if ((mTotalSize + neededSpace) < mMaxCacheSizeInBytes) {
/** Prunes the cache to fit the maximum size. */
private void pruneIfNeeded() {
if (mTotalSize < mMaxCacheSizeInBytes) {
return;
}
if (VolleyLog.DEBUG) {
Expand All @@ -288,7 +285,7 @@ private void pruneIfNeeded(int neededSpace) {
iterator.remove();
prunedFiles++;

if ((mTotalSize + neededSpace) < mMaxCacheSizeInBytes * HYSTERESIS_FACTOR) {
if (mTotalSize < mMaxCacheSizeInBytes * HYSTERESIS_FACTOR) {
break;
}
}
Expand Down Expand Up @@ -331,7 +328,7 @@ private void removeEntry(String key) {
* @param length number of bytes to read
* @throws IOException if fails to read all bytes
*/
// VisibleForTesting
@VisibleForTesting
static byte[] streamToBytes(CountingInputStream cis, long length) throws IOException {
long maxLength = cis.bytesRemaining();
// Length cannot be negative or greater than bytes remaining, and must not overflow int.
Expand All @@ -343,20 +340,24 @@ static byte[] streamToBytes(CountingInputStream cis, long length) throws IOExcep
return bytes;
}

// VisibleForTesting
@VisibleForTesting
InputStream createInputStream(File file) throws FileNotFoundException {
return new FileInputStream(file);
}

// VisibleForTesting
@VisibleForTesting
OutputStream createOutputStream(File file) throws FileNotFoundException {
return new FileOutputStream(file);
}

/** Handles holding onto the cache headers for an entry. */
// VisibleForTesting
@VisibleForTesting
static class CacheHeader {
/** The size of the data identified by this CacheHeader. (This is not serialized to disk. */
/**
* The size of the data identified by this CacheHeader on disk (both header and data).
*
* <p>Must be set by the caller after it has been calculated.
*/
long size;

/** The key that identifies the cache entry. */
Expand Down Expand Up @@ -412,7 +413,6 @@ private CacheHeader(
entry.ttl,
entry.softTtl,
getAllResponseHeaders(entry));
size = entry.data.length;
}

private static List<Header> getAllResponseHeaders(Entry entry) {
Expand Down
121 changes: 103 additions & 18 deletions src/test/java/com/android/volley/toolbox/DiskBasedCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,36 +166,110 @@ public void testInvalidateFullExpire() {
}

@Test
public void testTrim() {
Cache.Entry entry = randomData(2 * MAX_SIZE);
public void testTooLargeEntry() {
Cache.Entry entry = randomData(MAX_SIZE - getEntrySizeOnDisk("oversize"));
cache.put("oversize", entry);

assertThatEntriesAreEqual(cache.get("oversize"), entry);
assertThat(cache.get("oversize"), is(nullValue()));
}

@Test
public void testMaxSizeEntry() {
Cache.Entry entry = randomData(MAX_SIZE - getEntrySizeOnDisk("maxsize") - 1);
cache.put("maxsize", entry);

entry = randomData(1024);
cache.put("kilobyte", entry);
assertThatEntriesAreEqual(cache.get("maxsize"), entry);
}

assertThat(cache.get("oversize"), is(nullValue()));
assertThatEntriesAreEqual(cache.get("kilobyte"), entry);
@Test
public void testTrimAtThreshold() {
// Start with the largest possible entry.
Cache.Entry entry = randomData(MAX_SIZE - getEntrySizeOnDisk("maxsize") - 1);
cache.put("maxsize", entry);

assertThatEntriesAreEqual(cache.get("maxsize"), entry);

Cache.Entry entry2 = randomData(1024);
cache.put("kilobyte2", entry2);
Cache.Entry entry3 = randomData(1024);
cache.put("kilobyte3", entry3);
// Now any new entry should cause the first one to be cleared.
entry = randomData(0);
cache.put("bit", entry);

assertThatEntriesAreEqual(cache.get("kilobyte"), entry);
assertThatEntriesAreEqual(cache.get("kilobyte2"), entry2);
assertThatEntriesAreEqual(cache.get("kilobyte3"), entry3);
assertThat(cache.get("goodsize"), is(nullValue()));
assertThatEntriesAreEqual(cache.get("bit"), entry);
}

entry = randomData(MAX_SIZE);
@Test
public void testTrimWithMultipleEvictions_underHysteresisThreshold() {
Cache.Entry entry1 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry1") - 1);
cache.put("entry1", entry1);
Cache.Entry entry2 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry2") - 1);
cache.put("entry2", entry2);
Cache.Entry entry3 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry3") - 1);
cache.put("entry3", entry3);

assertThatEntriesAreEqual(cache.get("entry1"), entry1);
assertThatEntriesAreEqual(cache.get("entry2"), entry2);
assertThatEntriesAreEqual(cache.get("entry3"), entry3);

Cache.Entry entry =
randomData(
(int) (DiskBasedCache.HYSTERESIS_FACTOR * MAX_SIZE)
- getEntrySizeOnDisk("max"));
cache.put("max", entry);

assertThat(cache.get("kilobyte"), is(nullValue()));
assertThat(cache.get("kilobyte2"), is(nullValue()));
assertThat(cache.get("kilobyte3"), is(nullValue()));
assertThat(cache.get("entry1"), is(nullValue()));
assertThat(cache.get("entry2"), is(nullValue()));
assertThat(cache.get("entry3"), is(nullValue()));
assertThatEntriesAreEqual(cache.get("max"), entry);
}

@Test
public void testTrimWithMultipleEvictions_atHysteresisThreshold() {
Cache.Entry entry1 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry1") - 1);
cache.put("entry1", entry1);
Cache.Entry entry2 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry2") - 1);
cache.put("entry2", entry2);
Cache.Entry entry3 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry3") - 1);
cache.put("entry3", entry3);

assertThatEntriesAreEqual(cache.get("entry1"), entry1);
assertThatEntriesAreEqual(cache.get("entry2"), entry2);
assertThatEntriesAreEqual(cache.get("entry3"), entry3);

Cache.Entry entry =
randomData(
(int) (DiskBasedCache.HYSTERESIS_FACTOR * MAX_SIZE)
- getEntrySizeOnDisk("max")
+ 1);
cache.put("max", entry);

assertThat(cache.get("entry1"), is(nullValue()));
assertThat(cache.get("entry2"), is(nullValue()));
assertThat(cache.get("entry3"), is(nullValue()));
assertThat(cache.get("max"), is(nullValue()));
}

@Test
public void testTrimWithPartialEvictions() {
Cache.Entry entry1 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry1") - 1);
cache.put("entry1", entry1);
Cache.Entry entry2 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry2") - 1);
cache.put("entry2", entry2);
Cache.Entry entry3 = randomData(MAX_SIZE / 3 - getEntrySizeOnDisk("entry3") - 1);
cache.put("entry3", entry3);

assertThatEntriesAreEqual(cache.get("entry1"), entry1);
assertThatEntriesAreEqual(cache.get("entry2"), entry2);
assertThatEntriesAreEqual(cache.get("entry3"), entry3);

Cache.Entry entry4 = randomData((MAX_SIZE - getEntrySizeOnDisk("entry4") - 1) / 2);
cache.put("entry4", entry4);

assertThat(cache.get("entry1"), is(nullValue()));
assertThat(cache.get("entry2"), is(nullValue()));
assertThatEntriesAreEqual(cache.get("entry3"), entry3);
assertThatEntriesAreEqual(cache.get("entry4"), entry4);
}

@Test
@SuppressWarnings("TryFinallyCanBeTryWithResources")
public void testGetBadMagic() throws IOException {
Expand Down Expand Up @@ -518,4 +592,15 @@ private Cache.Entry randomData(int length) {
private File[] listCachedFiles() {
return temporaryFolder.getRoot().listFiles();
}

private int getEntrySizeOnDisk(String key) {
// Header size is:
// 4 bytes for magic int
// 8 + len(key) bytes for key (long length)
// 8 bytes for etag (long length + 0 characters)
// 32 bytes for serverDate, lastModified, ttl, and softTtl longs
// 4 bytes for length of header list int
// == 56 + len(key) bytes total.
return 56 + key.length();
}
}

0 comments on commit 1476fac

Please sign in to comment.