Make the HttpResponseCache robust to file I/O errors.
If the filesystem fails, cache performance degrades. Ongoing
writes should not see any filesystem errors: don't punish the
writer who isn't benefitting from the cache. Ongoing reads may
see IOExceptions while streaming files. In practice this will
only happen if the directory disappears during use, as is the
case when a removable partition is removed.
Change-Id: Ibf4d51998d9beaba9f8c86c449fd5c97ca869cee
http://b/3180373
diff --git a/luni/src/main/java/libcore/io/DiskLruCache.java b/luni/src/main/java/libcore/io/DiskLruCache.java
index d0a2797..c887541 100644
--- a/luni/src/main/java/libcore/io/DiskLruCache.java
+++ b/luni/src/main/java/libcore/io/DiskLruCache.java
@@ -80,12 +80,14 @@
* <p>Clients call {@link #get} to read a snapshot of an entry. The read will
* observe the value at the time that {@link #get} was called. Updates and
* removals after the call do not impact ongoing reads.
+ *
+ * <p>This class is tolerant of some I/O errors. If files are missing from the
+ * filesystem, the corresponding entries will be dropped from the cache. If
+ * an error occurs while writing a cache value, the edit will fail silently.
+ * Callers should handle other problems by catching {@code IOException} and
+ * responding appropriately.
*/
public final class DiskLruCache implements Closeable {
- // TODO: test with fault injection
- // TODO: a full disk should fail quietly!
- // TODO: missing individual files should fail quietly
-
static final String JOURNAL_FILE = "journal";
static final String JOURNAL_FILE_TMP = "journal.tmp";
static final String MAGIC = "libcore.io.DiskLruCache";
@@ -713,7 +715,7 @@
@Override public void write(int oneByte) {
try {
- super.write(oneByte);
+ out.write(oneByte);
} catch (IOException e) {
hasErrors = true;
}
@@ -721,7 +723,7 @@
@Override public void write(byte[] buffer, int offset, int length) {
try {
- super.write(buffer, offset, length);
+ out.write(buffer, offset, length);
} catch (IOException e) {
hasErrors = true;
}
@@ -729,7 +731,7 @@
@Override public void close() {
try {
- super.close();
+ out.close();
} catch (IOException e) {
hasErrors = true;
}
@@ -737,7 +739,7 @@
@Override public void flush() {
try {
- super.flush();
+ out.flush();
} catch (IOException e) {
hasErrors = true;
}
diff --git a/luni/src/main/java/libcore/net/http/HttpResponseCache.java b/luni/src/main/java/libcore/net/http/HttpResponseCache.java
index 3a66ef0..50ad5e2 100644
--- a/luni/src/main/java/libcore/net/http/HttpResponseCache.java
+++ b/luni/src/main/java/libcore/net/http/HttpResponseCache.java
@@ -59,8 +59,7 @@
* this.
*/
public final class HttpResponseCache extends ResponseCache {
- // TODO: add APIs to iterate the cache
-
+ // TODO: add APIs to iterate the cache?
private static final int VERSION = 201105;
private static final int ENTRY_METADATA = 0;
private static final int ENTRY_BODY = 1;
@@ -90,15 +89,21 @@
}
@Override public CacheResponse get(URI uri, String requestMethod,
- Map<String, List<String>> requestHeaders) throws IOException {
+ Map<String, List<String>> requestHeaders) {
String key = uriToKey(uri);
- DiskLruCache.Snapshot snapshot = cache.get(key);
-
- if (snapshot == null) {
+ DiskLruCache.Snapshot snapshot;
+ Entry entry;
+ try {
+ snapshot = cache.get(key);
+ if (snapshot == null) {
+ return null;
+ }
+ entry = new Entry(new BufferedInputStream(snapshot.getInputStream(ENTRY_METADATA)));
+ } catch (IOException e) {
+ // Give up because the cache cannot be read.
return null;
}
- Entry entry = new Entry(new BufferedInputStream(snapshot.getInputStream(ENTRY_METADATA)));
if (!entry.matches(uri, requestMethod, requestHeaders)) {
snapshot.close();
return null;
@@ -132,19 +137,21 @@
String requestMethod = httpConnection.getRequestMethod();
String key = uriToKey(uri);
- // Invalidate the cache on POST, PUT and DELETE.
if (requestMethod.equals(HttpEngine.POST)
|| requestMethod.equals(HttpEngine.PUT)
|| requestMethod.equals(HttpEngine.DELETE)) {
- cache.remove(key);
- }
-
- /*
- * Don't cache non-GET responses. We're technically allowed to cache
- * HEAD requests and some POST requests, but the complexity of doing so
- * is high and the benefit is low.
- */
- if (!requestMethod.equals(HttpEngine.GET)) {
+ try {
+ cache.remove(key);
+ } catch (IOException ignored) {
+ // The cache cannot be written.
+ }
+ return null;
+ } else if (!requestMethod.equals(HttpEngine.GET)) {
+ /*
+ * Don't cache non-GET responses. We're technically allowed to cache
+ * HEAD requests and some POST requests, but the complexity of doing
+ * so is high and the benefit is low.
+ */
return null;
}
@@ -159,11 +166,26 @@
return null;
}
- DiskLruCache.Editor editor = cache.edit(key);
RawHeaders varyHeaders = httpEngine.getRequestHeaders().headers.getAll(response.varyFields);
Entry entry = new Entry(uri, varyHeaders, httpConnection);
- entry.writeTo(editor);
- return new CacheRequestImpl(editor);
+ DiskLruCache.Editor editor = null;
+ try {
+ editor = cache.edit(key);
+ if (editor == null) {
+ return null;
+ }
+ entry.writeTo(editor);
+ return new CacheRequestImpl(editor);
+ } catch (IOException e) {
+ // Give up because the cache cannot be written.
+ try {
+ if (editor != null) {
+ editor.abort();
+ }
+ } catch (IOException ignored) {
+ }
+ return null;
+ }
}
private HttpEngine getHttpEngine(HttpURLConnection httpConnection) {
@@ -252,7 +274,7 @@
}
IoUtils.closeQuietly(cacheOut);
try {
- editor.abort(); // TODO: fix abort() to not throw?
+ editor.abort();
} catch (IOException ignored) {
}
}
diff --git a/luni/src/test/java/libcore/net/http/HttpResponseCacheTest.java b/luni/src/test/java/libcore/net/http/HttpResponseCacheTest.java
index 18e094d..a5673ff 100644
--- a/luni/src/test/java/libcore/net/http/HttpResponseCacheTest.java
+++ b/luni/src/test/java/libcore/net/http/HttpResponseCacheTest.java
@@ -24,6 +24,7 @@
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.OutputStream;
+import java.lang.reflect.InvocationHandler;
import java.net.CacheRequest;
import java.net.CacheResponse;
import java.net.HttpURLConnection;
@@ -41,6 +42,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
+import java.util.Deque;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
@@ -58,6 +60,7 @@
import tests.http.MockWebServer;
import tests.http.RecordedRequest;
import static tests.http.SocketPolicy.DISCONNECT_AT_END;
+import tests.io.MockOs;
public final class HttpResponseCacheTest extends TestCase {
@@ -66,6 +69,7 @@
private MockWebServer server = new MockWebServer();
private HttpResponseCache cache;
+ final MockOs mockOs = new MockOs();
@Override protected void setUp() throws Exception {
super.setUp();
@@ -74,9 +78,11 @@
File cacheDir = new File(tmp, "HttpCache-" + UUID.randomUUID());
cache = new HttpResponseCache(cacheDir, Integer.MAX_VALUE);
ResponseCache.setDefault(cache);
+ mockOs.install();
}
@Override protected void tearDown() throws Exception {
+ mockOs.uninstall();
server.shutdown();
ResponseCache.setDefault(null);
cache.getCache().delete();
@@ -1465,6 +1471,34 @@
assertEquals("A", readAscii(connection2));
}
+ public void testDiskWriteFailureCacheDegradation() throws Exception {
+ Deque<InvocationHandler> writeHandlers = mockOs.getHandlers("write");
+ int i = 0;
+ boolean hasMoreScenarios = true;
+ while (hasMoreScenarios) {
+ mockOs.enqueueNormal("write", i++);
+ mockOs.enqueueFault("write");
+ exercisePossiblyFaultyCache(false);
+ hasMoreScenarios = writeHandlers.isEmpty();
+ writeHandlers.clear();
+ }
+ System.out.println("Exercising the cache performs " + (i - 1) + " writes.");
+ }
+
+ public void testDiskReadFailureCacheDegradation() throws Exception {
+ Deque<InvocationHandler> readHandlers = mockOs.getHandlers("read");
+ int i = 0;
+ boolean hasMoreScenarios = true;
+ while (hasMoreScenarios) {
+ mockOs.enqueueNormal("read", i++);
+ mockOs.enqueueFault("read");
+ exercisePossiblyFaultyCache(true);
+ hasMoreScenarios = readHandlers.isEmpty();
+ readHandlers.clear();
+ }
+ System.out.println("Exercising the cache performs " + (i - 1) + " reads.");
+ }
+
/**
* @param delta the offset from the current date to use. Negative
* values yield dates in the past; positive values yield dates in the
@@ -1500,6 +1534,31 @@
assertEquals("B", readAscii(url.openConnection()));
}
+ private void exercisePossiblyFaultyCache(boolean permitReadBodyFailures) throws Exception {
+ server.shutdown();
+ server = new MockWebServer();
+ server.enqueue(new MockResponse()
+ .addHeader("Cache-Control: max-age=60")
+ .setBody("A"));
+ server.enqueue(new MockResponse().setBody("B"));
+ server.play();
+
+ URL url = server.getUrl("/" + UUID.randomUUID());
+ assertEquals("A", readAscii(url.openConnection()));
+
+ URLConnection connection = url.openConnection();
+ InputStream in = connection.getInputStream();
+ try {
+ int bodyChar = in.read();
+ assertTrue(bodyChar == 'A' || bodyChar == 'B');
+ assertEquals(-1, in.read());
+ } catch (IOException e) {
+ if (!permitReadBodyFailures) {
+ throw e;
+ }
+ }
+ }
+
/**
* @return the request with the conditional get headers.
*/
diff --git a/support/src/test/java/tests/io/MockOs.java b/support/src/test/java/tests/io/MockOs.java
index 46c43a7..0cfe836 100644
--- a/support/src/test/java/tests/io/MockOs.java
+++ b/support/src/test/java/tests/io/MockOs.java
@@ -20,9 +20,9 @@
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
-import java.util.ArrayList;
+import java.util.ArrayDeque;
+import java.util.Deque;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
import libcore.io.ErrnoException;
import libcore.io.Libcore;
@@ -34,35 +34,37 @@
* be useful to test otherwise hard-to-test scenarios such as a full disk.
*/
public final class MockOs {
- private final InheritableThreadLocal<Map<String, List<Fault>>> faults
- = new InheritableThreadLocal<Map<String, List<Fault>>>() {
- @Override protected Map<String, List<Fault>> initialValue() {
- return new HashMap<String, List<Fault>>();
+ private final InheritableThreadLocal<Map<String, Deque<InvocationHandler>>> handlers
+ = new InheritableThreadLocal<Map<String, Deque<InvocationHandler>>>() {
+ @Override protected Map<String, Deque<InvocationHandler>> initialValue() {
+ return new HashMap<String, Deque<InvocationHandler>>();
+ }
+ };
+
+ private Os delegate;
+ private final InvocationHandler delegateHandler = new InvocationHandler() {
+ @Override public Object invoke(Object o, Method method, Object[] args) throws Throwable {
+ try {
+ return method.invoke(delegate, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
}
};
private final InvocationHandler invocationHandler = new InvocationHandler() {
@Override public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable {
- Map<String, List<Fault>> threadFaults = faults.get();
- List<Fault> methodFaults = threadFaults.get(method.getName());
- if (methodFaults == null || methodFaults.isEmpty()) {
- try {
- return method.invoke(delegate, args);
- } catch (InvocationTargetException e) {
- throw e.getCause();
- }
+ InvocationHandler handler = getHandlers(method.getName()).poll();
+ if (handler == null) {
+ handler = delegateHandler;
}
-
- Fault fault = methodFaults.remove(0);
- fault.trigger(method);
- return null;
+ return handler.invoke(proxy, method, args);
}
};
private final Os mockOs = (Os) Proxy.newProxyInstance(MockOs.class.getClassLoader(),
new Class[] { Os.class }, invocationHandler);
- private Os delegate;
public void install() {
if (delegate != null) {
@@ -79,28 +81,40 @@
Libcore.os = delegate;
}
+ /**
+ * Returns the invocation handlers to handle upcoming invocations of
+ * {@code methodName}. If empty, calls will be handled by the delegate.
+ */
+ public Deque<InvocationHandler> getHandlers(String methodName) {
+ Map<String, Deque<InvocationHandler>> threadFaults = handlers.get();
+ Deque<InvocationHandler> result = threadFaults.get(methodName);
+ if (result == null) {
+ result = new ArrayDeque<InvocationHandler>();
+ threadFaults.put(methodName, result);
+ }
+ return result;
+ }
+
+ /**
+ * Enqueues the specified number of normal operations. Useful to delay
+ * faults.
+ */
+ public void enqueueNormal(String methodName, int count) {
+ Deque<InvocationHandler> handlers = getHandlers(methodName);
+ for (int i = 0; i < count; i++) {
+ handlers.add(delegateHandler);
+ }
+ }
+
public void enqueueFault(String methodName) {
enqueueFault(methodName, OsConstants.EIO);
}
- public void enqueueFault(String methodName, int errno) {
- Map<String, List<Fault>> threadFaults = faults.get();
- List<Fault> methodFaults = threadFaults.get(methodName);
- if (methodFaults == null) {
- methodFaults = new ArrayList<Fault>();
- threadFaults.put(methodName, methodFaults);
- }
- methodFaults.add(new Fault(errno));
- }
-
- private static class Fault {
- private final int errno;
- public Fault(int errno) {
- this.errno = errno;
- }
-
- public void trigger(Method method) {
- throw new ErrnoException(method.getName(), errno);
- }
+ public void enqueueFault(String methodName, final int errno) {
+ getHandlers(methodName).add(new InvocationHandler() {
+ @Override public Object invoke(Object proxy, Method method, Object[] args) {
+ throw new ErrnoException(method.getName(), errno);
+ }
+ });
}
}