From f6f3497ef7d01c2e11b68ca96b7f84e566e6c745 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sat, 18 Jan 2014 12:24:47 -0500 Subject: [PATCH] Cache SPDY responses even if the body isn't fully consumed. https://github.com/square/okhttp/issues/450 --- .../java/com/squareup/okhttp/Headers.java | 8 ++ .../http/AbstractHttpInputStream.java | 2 +- .../okhttp/internal/http/HttpTransport.java | 7 -- .../okhttp/internal/http/SpdyTransport.java | 84 +++++++++++++++---- .../okhttp/internal/http/Transport.java | 7 ++ .../internal/http/HttpOverSpdyTest.java | 22 ++++- 6 files changed, 106 insertions(+), 24 deletions(-) diff --git a/okhttp/src/main/java/com/squareup/okhttp/Headers.java b/okhttp/src/main/java/com/squareup/okhttp/Headers.java index 7753d9f1f..1221aa4e0 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/Headers.java +++ b/okhttp/src/main/java/com/squareup/okhttp/Headers.java @@ -119,6 +119,14 @@ public final class Headers { return result; } + @Override public String toString() { + StringBuilder result = new StringBuilder(); + for (int i = 0; i < size(); i++) { + result.append(name(i)).append(": ").append(value(i)).append("\n"); + } + return result.toString(); + } + private static String get(String[] namesAndValues, String fieldName) { for (int i = namesAndValues.length - 2; i >= 0; i -= 2) { if (fieldName.equalsIgnoreCase(namesAndValues[i])) { diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/AbstractHttpInputStream.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/AbstractHttpInputStream.java index 612693644..a7a818e2e 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/AbstractHttpInputStream.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/AbstractHttpInputStream.java @@ -36,7 +36,7 @@ abstract class AbstractHttpInputStream extends InputStream { protected final InputStream in; protected final HttpEngine httpEngine; private final CacheRequest cacheRequest; - private final OutputStream cacheBody; + protected final OutputStream cacheBody; protected boolean closed; AbstractHttpInputStream(InputStream in, HttpEngine httpEngine, CacheRequest cacheRequest) diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java index c516b4770..51810923a 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpTransport.java @@ -35,13 +35,6 @@ import static com.squareup.okhttp.internal.Util.checkOffsetAndCount; import static com.squareup.okhttp.internal.http.StatusLine.HTTP_CONTINUE; public final class HttpTransport implements Transport { - /** - * The timeout to use while discarding a stream of input data. Since this is - * used for connection reuse, this timeout should be significantly less than - * the time it takes to establish a new connection. - */ - private static final int DISCARD_STREAM_TIMEOUT_MILLIS = 100; - public static final int DEFAULT_CHUNK_LENGTH = 1024; private final HttpEngine httpEngine; diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java index 4c1e80457..042bbf9e7 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/SpdyTransport.java @@ -17,10 +17,11 @@ package com.squareup.okhttp.internal.http; import com.squareup.okhttp.Headers; +import com.squareup.okhttp.Protocol; import com.squareup.okhttp.Request; import com.squareup.okhttp.Response; import com.squareup.okhttp.internal.ByteString; -import com.squareup.okhttp.Protocol; +import com.squareup.okhttp.internal.Util; import com.squareup.okhttp.internal.spdy.ErrorCode; import com.squareup.okhttp.internal.spdy.SpdyConnection; import com.squareup.okhttp.internal.spdy.SpdyStream; @@ -35,6 +36,8 @@ import java.util.List; import java.util.Locale; import java.util.Set; +import static com.squareup.okhttp.internal.Util.checkOffsetAndCount; + public final class SpdyTransport implements Transport { private static final ByteString HEADER_METHOD = ByteString.encodeUtf8(":method"); private static final ByteString HEADER_PATH = ByteString.encodeUtf8(":path"); @@ -191,23 +194,12 @@ public final class SpdyTransport implements Transport { } @Override public InputStream getTransferStream(CacheRequest cacheRequest) throws IOException { - return new UnknownLengthHttpInputStream(stream.getInputStream(), cacheRequest, httpEngine); + return new SpdyInputStream(stream, cacheRequest, httpEngine); } @Override public boolean makeReusable(boolean streamCanceled, OutputStream requestBodyOut, InputStream responseBodyIn) { - if (streamCanceled) { - if (stream != null) { - stream.closeLater(ErrorCode.CANCEL); - return true; - } else { - // If stream is null, it either means that writeRequestHeaders wasn't called - // or that SpdyConnection#newStream threw an IOException. In both cases there's - // nothing to do here and this stream can't be reused. - return false; - } - } - return true; + return true; // SPDY sockets are always reusable. } /** When true, this header should not be emitted or consumed. */ @@ -239,4 +231,68 @@ public final class SpdyTransport implements Transport { } return prohibited; } + + /** An HTTP message body terminated by the end of the underlying stream. */ + private static class SpdyInputStream extends AbstractHttpInputStream { + private final SpdyStream stream; + private boolean inputExhausted; + + SpdyInputStream(SpdyStream stream, CacheRequest cacheRequest, HttpEngine httpEngine) + throws IOException { + super(stream.getInputStream(), httpEngine, cacheRequest); + this.stream = stream; + } + + @Override public int read(byte[] buffer, int offset, int count) throws IOException { + checkOffsetAndCount(buffer.length, offset, count); + checkNotClosed(); + if (in == null || inputExhausted) { + return -1; + } + int read = in.read(buffer, offset, count); + if (read == -1) { + inputExhausted = true; + endOfInput(); + return -1; + } + cacheWrite(buffer, offset, read); + return read; + } + + @Override public int available() throws IOException { + checkNotClosed(); + return in == null ? 0 : in.available(); + } + + @Override public void close() throws IOException { + if (closed) return; + + if (!inputExhausted && cacheBody != null) { + discardStream(); // Could make inputExhausted true! + } + + closed = true; + + if (!inputExhausted) { + stream.closeLater(ErrorCode.CANCEL); + unexpectedEndOfInput(); + } + } + + private boolean discardStream() { + try { + long socketTimeout = stream.getReadTimeoutMillis(); + stream.setReadTimeout(socketTimeout); + stream.setReadTimeout(DISCARD_STREAM_TIMEOUT_MILLIS); + try { + Util.skipByReading(this, Long.MAX_VALUE, DISCARD_STREAM_TIMEOUT_MILLIS); + return true; + } finally { + stream.setReadTimeout(socketTimeout); + } + } catch (IOException e) { + return false; + } + } + } } diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java index 59e986f1a..b1c38fec5 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/Transport.java @@ -24,6 +24,13 @@ import java.io.OutputStream; import java.net.CacheRequest; interface Transport { + /** + * The timeout to use while discarding a stream of input data. Since this is + * used for connection reuse, this timeout should be significantly less than + * the time it takes to establish a new connection. + */ + int DISCARD_STREAM_TIMEOUT_MILLIS = 100; + /** * Returns an output stream where the request body can be written. The * returned stream will of one of two types: diff --git a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java index 223fb8e1a..8cc7bfd70 100644 --- a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java +++ b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java @@ -282,7 +282,7 @@ public abstract class HttpOverSpdyTest { } @Test public void responsesAreCached() throws IOException { - client.setResponseCache(cache); + client.setOkResponseCache(cache); server.enqueue(new MockResponse().addHeader("cache-control: max-age=60").setBody("A")); server.play(); @@ -299,7 +299,7 @@ public abstract class HttpOverSpdyTest { } @Test public void conditionalCache() throws IOException { - client.setResponseCache(cache); + client.setOkResponseCache(cache); server.enqueue(new MockResponse().addHeader("ETag: v1").setBody("A")); server.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_NOT_MODIFIED)); @@ -315,6 +315,24 @@ public abstract class HttpOverSpdyTest { assertEquals(1, cache.getHitCount()); } + @Test public void responseCachedWithoutConsumingFullBody() throws IOException { + client.setOkResponseCache(cache); + + server.enqueue(new MockResponse().addHeader("cache-control: max-age=60").setBody("ABCD")); + server.enqueue(new MockResponse().addHeader("cache-control: max-age=60").setBody("EFGH")); + server.play(); + + URLConnection connection1 = client.open(server.getUrl("/")); + InputStream in1 = connection1.getInputStream(); + assertEquals("AB", readAscii(in1, 2)); + in1.close(); + + URLConnection connection2 = client.open(server.getUrl("/")); + InputStream in2 = connection2.getInputStream(); + assertEquals("ABCD", readAscii(in2, Integer.MAX_VALUE)); + in2.close(); + } + @Test public void acceptAndTransmitCookies() throws Exception { CookieManager cookieManager = new CookieManager(); client.setCookieHandler(cookieManager);