Merge pull request #59 from square/jwilson/improve_reuse

Improve connection reuse.

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;
         }
     }
 

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"));

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()