From aae1a45bcb6dd2ffd3b0b87525bbef4e4517ddba Mon Sep 17 00:00:00 2001 From: jwilson Date: Sun, 20 Mar 2016 17:22:20 -0400 Subject: [PATCH] Recover more gracefully when an editor is detached. We had a bug where we could have two editors for the same entry. This would occur when the cache was cleared, or if the entry was otherwise evicted while the edit was in progress. Previously the two editors would corrupt each other, and potentially the cache's size. With this change the detached editor is limited and harmless. --- .../okhttp3/internal/DiskLruCacheTest.java | 74 +++++++++++++++++++ .../java/okhttp3/internal/DiskLruCache.java | 55 ++++++++++---- 2 files changed, 115 insertions(+), 14 deletions(-) diff --git a/okhttp-tests/src/test/java/okhttp3/internal/DiskLruCacheTest.java b/okhttp-tests/src/test/java/okhttp3/internal/DiskLruCacheTest.java index 3e7c9771c..be2d0be61 100644 --- a/okhttp-tests/src/test/java/okhttp3/internal/DiskLruCacheTest.java +++ b/okhttp-tests/src/test/java/okhttp3/internal/DiskLruCacheTest.java @@ -1536,6 +1536,80 @@ public final class DiskLruCacheTest { assertNull(cache.get("a")); } + @Test public void noSizeCorruptionAfterCreatorDetached() throws Exception { + // Create an editor for k1. Detach it by clearing the cache. + DiskLruCache.Editor editor = cache.edit("k1"); + setString(editor, 0, "a"); + setString(editor, 1, "a"); + cache.evictAll(); + + // Create a new value in its place. + set("k1", "bb", "bb"); + assertEquals(4, cache.size()); + + // Committing the detached editor should not change the cache's size. + editor.commit(); + assertEquals(4, cache.size()); + assertValue("k1", "bb", "bb"); + } + + @Test public void noSizeCorruptionAfterEditorDetached() throws Exception { + set("k1", "a", "a"); + + // Create an editor for k1. Detach it by clearing the cache. + DiskLruCache.Editor editor = cache.edit("k1"); + setString(editor, 0, "bb"); + setString(editor, 1, "bb"); + cache.evictAll(); + + // Create a new value in its place. + set("k1", "ccc", "ccc"); + assertEquals(6, cache.size()); + + // Committing the detached editor should not change the cache's size. + editor.commit(); + assertEquals(6, cache.size()); + assertValue("k1", "ccc", "ccc"); + } + + @Test public void noNewSourceAfterEditorDetached() throws Exception { + set("k1", "a", "a"); + + DiskLruCache.Editor editor = cache.edit("k1"); + cache.evictAll(); + + assertNull(editor.newSource(0)); + } + + @Test public void editsDiscardedAfterEditorDetached() throws Exception { + set("k1", "a", "a"); + + // Create an editor, then detach it. + DiskLruCache.Editor editor = cache.edit("k1"); + BufferedSink sink = Okio.buffer(editor.newSink(0)); + cache.evictAll(); + + // Create another value in its place. + set("k1", "ccc", "ccc"); + + // Complete the original edit. It goes into a black hole. + sink.writeUtf8("bb"); + sink.close(); + + assertValue("k1", "ccc", "ccc"); + } + + @Test public void abortAfterDetach() throws Exception { + set("k1", "a", "a"); + + DiskLruCache.Editor editor = cache.edit("k1"); + cache.evictAll(); + + editor.abort(); + assertEquals(0, cache.size()); + assertAbsent("k1"); + } + private void assertJournalEquals(String... expectedBodyLines) throws Exception { List expectedLines = new ArrayList<>(); expectedLines.add(MAGIC); diff --git a/okhttp/src/main/java/okhttp3/internal/DiskLruCache.java b/okhttp/src/main/java/okhttp3/internal/DiskLruCache.java index b7964baf9..51ce88e9f 100644 --- a/okhttp/src/main/java/okhttp3/internal/DiskLruCache.java +++ b/okhttp/src/main/java/okhttp3/internal/DiskLruCache.java @@ -612,7 +612,7 @@ public final class DiskLruCache implements Closeable, Flushable { private boolean removeEntry(Entry entry) throws IOException { if (entry.currentEditor != null) { - entry.currentEditor.hasErrors = true; // Prevent the edit from completing normally. + entry.currentEditor.detach(); // Prevent the edit from completing normally. } for (int i = 0; i < valueCount; i++) { @@ -838,24 +838,42 @@ public final class DiskLruCache implements Closeable, Flushable { public final class Editor { private final Entry entry; private final boolean[] written; - private boolean hasErrors; - private boolean committed; + private boolean done; private Editor(Entry entry) { this.entry = entry; this.written = (entry.readable) ? null : new boolean[valueCount]; } + /** + * Prevents this editor from completing normally. This is necessary either when the edit causes + * an I/O error, or if the target entry is evicted while this editor is active. In either case + * we delete the editor's created files and prevent new files from being created. Note that once + * an editor has been detached it is possible for another editor to edit the entry. + */ + void detach() { + if (entry.currentEditor == this) { + for (int i = 0; i < valueCount; i++) { + try { + fileSystem.delete(entry.dirtyFiles[i]); + } catch (IOException e) { + // This file is potentially leaked. Not much we can do about that. + } + } + entry.currentEditor = null; + } + } + /** * Returns an unbuffered input stream to read the last committed value, or null if no value has * been committed. */ public Source newSource(int index) throws IOException { synchronized (DiskLruCache.this) { - if (entry.currentEditor != this) { + if (done) { throw new IllegalStateException(); } - if (!entry.readable) { + if (!entry.readable || entry.currentEditor != this) { return null; } try { @@ -873,9 +891,12 @@ public final class DiskLruCache implements Closeable, Flushable { */ public Sink newSink(int index) throws IOException { synchronized (DiskLruCache.this) { - if (entry.currentEditor != this) { + if (done) { throw new IllegalStateException(); } + if (entry.currentEditor != this) { + return NULL_SINK; + } if (!entry.readable) { written[index] = true; } @@ -889,7 +910,7 @@ public final class DiskLruCache implements Closeable, Flushable { return new FaultHidingSink(sink) { @Override protected void onException(IOException e) { synchronized (DiskLruCache.this) { - hasErrors = true; + detach(); } } }; @@ -902,13 +923,13 @@ public final class DiskLruCache implements Closeable, Flushable { */ public void commit() throws IOException { synchronized (DiskLruCache.this) { - if (hasErrors) { - completeEdit(this, false); - removeEntry(entry); // The previous entry is stale. - } else { + if (done) { + throw new IllegalStateException(); + } + if (entry.currentEditor == this) { completeEdit(this, true); } - committed = true; + done = true; } } @@ -918,13 +939,19 @@ public final class DiskLruCache implements Closeable, Flushable { */ public void abort() throws IOException { synchronized (DiskLruCache.this) { - completeEdit(this, false); + if (done) { + throw new IllegalStateException(); + } + if (entry.currentEditor == this) { + completeEdit(this, false); + } + done = true; } } public void abortUnlessCommitted() { synchronized (DiskLruCache.this) { - if (!committed) { + if (!done && entry.currentEditor == this) { try { completeEdit(this, false); } catch (IOException ignored) {