From 04d742bd3d6ce8ad24b21f2be2d6f3ea5f8a35a5 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Thu, 6 Dec 2012 21:06:45 -0500 Subject: [PATCH] Improve connection reuse. Instead of trying to read the last bytes of a content body when the input stream is read, do it when the connection is closed. This approach is less elegant, since it requires a connection to do work on behalf of a potentially nonexistent follow up connection. But it should result in more reuse in practice. http://code.google.com/p/android/issues/detail?id=38817 --- .../java/libcore/net/http/HttpTransport.java | 61 +++++++++++-------- .../net/http/HttpResponseCacheTest.java | 3 +- .../libcore/net/http/URLConnectionTest.java | 42 +++++++++++-- 3 files changed, 77 insertions(+), 29 deletions(-) diff --git a/src/main/java/libcore/net/http/HttpTransport.java b/src/main/java/libcore/net/http/HttpTransport.java index 631c44bd4..ae9a8bce6 100644 --- a/src/main/java/libcore/net/http/HttpTransport.java +++ b/src/main/java/libcore/net/http/HttpTransport.java @@ -24,6 +24,7 @@ import java.io.OutputStream; import java.net.CacheRequest; import java.net.CookieHandler; import java.net.ProtocolException; +import java.net.Socket; import libcore.io.Streams; import libcore.util.Libcore; @@ -36,6 +37,13 @@ final class HttpTransport implements Transport { */ private static final int MAX_REQUEST_BUFFER_LENGTH = 32768; + /** + * 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 = 30; + private final HttpEngine httpEngine; private final InputStream socketIn; private final OutputStream socketOut; @@ -169,17 +177,36 @@ final class HttpTransport implements Transport { } if (responseBodyIn != null) { - // Discard the response body before the connection can be reused. - try { - Streams.skipAll(responseBodyIn); - } catch (IOException e) { - return false; - } + return discardStream(httpEngine, responseBodyIn); } return true; } + /** + * Discards the response body so that the connection can be reused. This + * needs to be done judiciously, since it delays the current request in + * order to speed up a potential future request that may never occur. + */ + private static boolean discardStream(HttpEngine httpEngine, InputStream responseBodyIn) { + HttpConnection connection = httpEngine.connection; + if (connection == null) return false; + Socket socket = connection.getSocket(); + if (socket == null) return false; + try { + int socketTimeout = socket.getSoTimeout(); + socket.setSoTimeout(DISCARD_STREAM_TIMEOUT_MILLIS); + try { + Streams.skipAll(responseBodyIn); + return true; + } finally { + socket.setSoTimeout(socketTimeout); + } + } catch (IOException e) { + return false; + } + } + @Override public InputStream getTransferStream(CacheRequest cacheRequest) throws IOException { if (!httpEngine.hasResponseBody()) { return new FixedLengthInputStream(socketIn, cacheRequest, httpEngine, 0); @@ -396,10 +423,10 @@ final class HttpTransport implements Transport { if (closed) { return; } - closed = true; - if (bytesRemaining != 0) { + if (bytesRemaining != 0 && !discardStream(httpEngine, this)) { unexpectedEndOfInput(); } + closed = true; } } @@ -407,7 +434,6 @@ final class HttpTransport implements Transport { * An HTTP body with alternating chunk sizes and chunk bodies. */ private static class ChunkedInputStream extends AbstractHttpInputStream { - private static final int MIN_LAST_CHUNK_LENGTH = "\r\n0\r\n\r\n".length(); private static final int NO_CHUNK_YET = -1; private final HttpTransport transport; private int bytesRemainingInChunk = NO_CHUNK_YET; @@ -439,18 +465,6 @@ final class HttpTransport implements Transport { } bytesRemainingInChunk -= read; cacheWrite(buffer, offset, read); - - /* - * If we're at the end of a chunk and the next chunk size is readable, - * read it! Reading the last chunk causes the underlying connection to - * be recycled and we want to do that as early as possible. Otherwise - * self-delimiting streams like gzip will never be recycled. - * http://code.google.com/p/android/issues/detail?id=7059 - */ - if (bytesRemainingInChunk == 0 && in.available() >= MIN_LAST_CHUNK_LENGTH) { - readChunkSize(); - } - return read; } @@ -490,11 +504,10 @@ final class HttpTransport implements Transport { if (closed) { return; } - - closed = true; - if (hasMoreChunks) { + if (hasMoreChunks && !discardStream(httpEngine, this)) { unexpectedEndOfInput(); } + closed = true; } } diff --git a/src/test/java/libcore/net/http/HttpResponseCacheTest.java b/src/test/java/libcore/net/http/HttpResponseCacheTest.java index 5c675facf..2094e9ecc 100644 --- a/src/test/java/libcore/net/http/HttpResponseCacheTest.java +++ b/src/test/java/libcore/net/http/HttpResponseCacheTest.java @@ -506,7 +506,8 @@ public final class HttpResponseCacheTest extends TestCase { } private void testClientPrematureDisconnect(TransferKind transferKind) throws IOException { - MockResponse response = new MockResponse(); + // Setting a low transfer speed ensures that stream discarding will time out. + MockResponse response = new MockResponse().setBytesPerSecond(6); transferKind.setBody(response, "ABCDE\nFGHIJKLMNOPQRSTUVWXYZ", 1024); server.enqueue(response); server.enqueue(new MockResponse().setBody("Request #2")); diff --git a/src/test/java/libcore/net/http/URLConnectionTest.java b/src/test/java/libcore/net/http/URLConnectionTest.java index 48a6a5dcf..075059de1 100644 --- a/src/test/java/libcore/net/http/URLConnectionTest.java +++ b/src/test/java/libcore/net/http/URLConnectionTest.java @@ -20,10 +20,6 @@ import com.google.mockwebserver.MockResponse; import com.google.mockwebserver.MockWebServer; import com.google.mockwebserver.RecordedRequest; import com.google.mockwebserver.SocketPolicy; -import static com.google.mockwebserver.SocketPolicy.DISCONNECT_AT_END; -import static com.google.mockwebserver.SocketPolicy.DISCONNECT_AT_START; -import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_INPUT_AT_END; -import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_OUTPUT_AT_END; import com.squareup.okhttp.OkHttpConnection; import com.squareup.okhttp.OkHttpsConnection; import java.io.ByteArrayOutputStream; @@ -75,6 +71,11 @@ import javax.net.ssl.X509TrustManager; import junit.framework.TestCase; import libcore.net.ssl.SslContextBuilder; +import static com.google.mockwebserver.SocketPolicy.DISCONNECT_AT_END; +import static com.google.mockwebserver.SocketPolicy.DISCONNECT_AT_START; +import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_INPUT_AT_END; +import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_OUTPUT_AT_END; + /** * Android's URLConnectionTest. */ @@ -1181,6 +1182,39 @@ public final class URLConnectionTest extends TestCase { assertEquals(1, server.takeRequest().getSequenceNumber()); } + public void testEarlyDisconnectDoesntHarmPoolingWithChunkedEncoding() throws Exception { + testEarlyDisconnectDoesntHarmPooling(TransferKind.CHUNKED); + } + + public void testEarlyDisconnectDoesntHarmPoolingWithFixedLengthEncoding() throws Exception { + testEarlyDisconnectDoesntHarmPooling(TransferKind.FIXED_LENGTH); + } + + private void testEarlyDisconnectDoesntHarmPooling(TransferKind transferKind) throws Exception { + MockResponse response1 = new MockResponse(); + transferKind.setBody(response1, "ABCDEFGHIJK", 1024); + server.enqueue(response1); + + MockResponse response2 = new MockResponse(); + transferKind.setBody(response2, "LMNOPQRSTUV", 1024); + server.enqueue(response2); + + server.play(); + + URLConnection connection1 = openConnection(server.getUrl("/")); + InputStream in1 = connection1.getInputStream(); + assertEquals("ABCDE", readAscii(in1, 5)); + in1.close(); + + OkHttpConnection connection2 = openConnection(server.getUrl("/")); + InputStream in2 = connection2.getInputStream(); + assertEquals("LMNOP", readAscii(in2, 5)); + in2.close(); + + assertEquals(0, server.takeRequest().getSequenceNumber()); + assertEquals(1, server.takeRequest().getSequenceNumber()); // Connection is pooled! + } + /** * Obnoxiously test that the chunk sizes transmitted exactly equal the * requested data+chunk header size. Although setChunkedStreamingMode()