fix: when disconnecting, close the underlying connection before the response InputStream (#1315)

Adds a test to the `NetHttpTransport` and `ApacheHttpTransport` to make sure we don't wait until all the content is read when disconnecting the response.

Fixes #1303

Author: chingor13

google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java

@@ -35,6 +35,8 @@
 import java.io.OutputStream;
 import java.net.InetSocketAddress;
 import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.http.Header;
@@ -213,11 +215,32 @@ public void testConnectTimeout() {
     }
   }
 
+  static class FakeServer implements AutoCloseable {
+    private final HttpServer server;
+    private final ExecutorService executorService;
+
+    public FakeServer(HttpHandler httpHandler) throws IOException {
+      this.server = HttpServer.create(new InetSocketAddress(0), 0);
+      this.executorService = Executors.newFixedThreadPool(1);
+      server.setExecutor(this.executorService);
+      server.createContext("/", httpHandler);
+      server.start();
+    }
+
+    public int getPort() {
+      return server.getAddress().getPort();
+    }
+
+    @Override
+    public void close() {
+      this.server.stop(0);
+      this.executorService.shutdownNow();
+    }
+  }
+
   @Test
   public void testNormalizedUrl() throws IOException {
-    HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
-    server.createContext(
-        "/",
+    final HttpHandler handler =
         new HttpHandler() {
           @Override
           public void handle(HttpExchange httpExchange) throws IOException {
@@ -227,19 +250,53 @@ public void handle(HttpExchange httpExchange) throws IOException {
               out.write(response);
             }
           }
-        });
-    server.start();
-
-    ApacheHttpTransport transport = new ApacheHttpTransport();
-    GenericUrl testUrl = new GenericUrl("http://localhost/foo//bar");
-    testUrl.setPort(server.getAddress().getPort());
-    com.google.api.client.http.HttpResponse response =
-        transport.createRequestFactory().buildGetRequest(testUrl).execute();
-    assertEquals(200, response.getStatusCode());
-    assertEquals("/foo//bar", response.parseAsString());
+        };
+    try (FakeServer server = new FakeServer(handler)) {
+      HttpTransport transport = new ApacheHttpTransport();
+      GenericUrl testUrl = new GenericUrl("http://localhost/foo//bar");
+      testUrl.setPort(server.getPort());
+      com.google.api.client.http.HttpResponse response =
+          transport.createRequestFactory().buildGetRequest(testUrl).execute();
+      assertEquals(200, response.getStatusCode());
+      assertEquals("/foo//bar", response.parseAsString());
+    }
   }
 
   private boolean isWindows() {
     return System.getProperty("os.name").startsWith("Windows");
   }
+
+  @Test(timeout = 10_000L)
+  public void testDisconnectShouldNotWaitToReadResponse() throws IOException {
+    // This handler waits for 100s before returning writing content. The test should
+    // timeout if disconnect waits for the response before closing the connection.
+    final HttpHandler handler =
+        new HttpHandler() {
+          @Override
+          public void handle(HttpExchange httpExchange) throws IOException {
+            byte[] response = httpExchange.getRequestURI().toString().getBytes();
+            httpExchange.sendResponseHeaders(200, response.length);
+
+            // Sleep for longer than the test timeout
+            try {
+              Thread.sleep(100_000);
+            } catch (InterruptedException e) {
+              throw new IOException("interrupted", e);
+            }
+            try (OutputStream out = httpExchange.getResponseBody()) {
+              out.write(response);
+            }
+          }
+        };
+
+    try (FakeServer server = new FakeServer(handler)) {
+      HttpTransport transport = new ApacheHttpTransport();
+      GenericUrl testUrl = new GenericUrl("http://localhost/foo//bar");
+      testUrl.setPort(server.getPort());
+      com.google.api.client.http.HttpResponse response =
+          transport.createRequestFactory().buildGetRequest(testUrl).execute();
+      // disconnect should not wait to read the entire content
+      response.disconnect();
+    }
+  }
 }

google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java

@@ -351,9 +351,9 @@ public InputStream getContent() throws IOException {
         try {
           // gzip encoding (wrap content with GZipInputStream)
           if (!returnRawInputStream && this.contentEncoding != null) {
-            String oontentencoding = this.contentEncoding.trim().toLowerCase(Locale.ENGLISH);
-            if (CONTENT_ENCODING_GZIP.equals(oontentencoding)
-                || CONTENT_ENCODING_XGZIP.equals(oontentencoding)) {
+            String contentEncoding = this.contentEncoding.trim().toLowerCase(Locale.ENGLISH);
+            if (CONTENT_ENCODING_GZIP.equals(contentEncoding)
+                || CONTENT_ENCODING_XGZIP.equals(contentEncoding)) {
               // Wrap the original stream in a ConsumingInputStream before passing it to
               // GZIPInputStream. The GZIPInputStream leaves content unconsumed in the original
               // stream (it almost always leaves the last chunk unconsumed in chunked responses).
@@ -419,9 +419,12 @@ public void download(OutputStream outputStream) throws IOException {
 
   /** Closes the content of the HTTP response from {@link #getContent()}, ignoring any content. */
   public void ignore() throws IOException {
-    InputStream content = getContent();
-    if (content != null) {
-      content.close();
+    if (this.response == null) {
+      return;
+    }
+    InputStream lowLevelResponseContent = this.response.getContent();
+    if (lowLevelResponseContent != null) {
+      lowLevelResponseContent.close();
     }
   }
 
@@ -432,8 +435,10 @@ public void ignore() throws IOException {
    * @since 1.4
    */
   public void disconnect() throws IOException {
-    ignore();
+    // Close the connection before trying to close the InputStream content. If you are trying to
+    // disconnect, we shouldn't need to try to read any further content.
     response.disconnect();
+    ignore();
   }
 
   /**

google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java

@@ -14,17 +14,27 @@
 
 package com.google.api.client.http.javanet;
 
+import com.google.api.client.http.GenericUrl;
+import com.google.api.client.http.HttpTransport;
 import com.google.api.client.testing.http.HttpTesting;
 import com.google.api.client.testing.http.javanet.MockHttpURLConnection;
 import com.google.api.client.util.ByteArrayStreamingContent;
 import com.google.api.client.util.StringUtils;
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpHandler;
+import com.sun.net.httpserver.HttpServer;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.HttpURLConnection;
+import java.net.InetSocketAddress;
 import java.net.URL;
 import java.security.KeyStore;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import junit.framework.TestCase;
+import org.junit.Test;
 
 /**
  * Tests {@link NetHttpTransport}.
@@ -159,4 +169,61 @@ private void setContent(NetHttpRequest request, String type, String value) throw
     request.setContentType(type);
     request.setContentLength(bytes.length);
   }
+
+  static class FakeServer implements AutoCloseable {
+    private final HttpServer server;
+    private final ExecutorService executorService;
+
+    public FakeServer(HttpHandler httpHandler) throws IOException {
+      this.server = HttpServer.create(new InetSocketAddress(0), 0);
+      this.executorService = Executors.newFixedThreadPool(1);
+      server.setExecutor(this.executorService);
+      server.createContext("/", httpHandler);
+      server.start();
+    }
+
+    public int getPort() {
+      return server.getAddress().getPort();
+    }
+
+    @Override
+    public void close() {
+      this.server.stop(0);
+      this.executorService.shutdownNow();
+    }
+  }
+
+  @Test(timeout = 10_000L)
+  public void testDisconnectShouldNotWaitToReadResponse() throws IOException {
+    // This handler waits for 100s before returning writing content. The test should
+    // timeout if disconnect waits for the response before closing the connection.
+    final HttpHandler handler =
+        new HttpHandler() {
+          @Override
+          public void handle(HttpExchange httpExchange) throws IOException {
+            byte[] response = httpExchange.getRequestURI().toString().getBytes();
+            httpExchange.sendResponseHeaders(200, response.length);
+
+            // Sleep for longer than the test timeout
+            try {
+              Thread.sleep(100_000);
+            } catch (InterruptedException e) {
+              throw new IOException("interrupted", e);
+            }
+            try (OutputStream out = httpExchange.getResponseBody()) {
+              out.write(response);
+            }
+          }
+        };
+
+    try (FakeServer server = new FakeServer(handler)) {
+      HttpTransport transport = new NetHttpTransport();
+      GenericUrl testUrl = new GenericUrl("http://localhost/foo//bar");
+      testUrl.setPort(server.getPort());
+      com.google.api.client.http.HttpResponse response =
+          transport.createRequestFactory().buildGetRequest(testUrl).execute();
+      // disconnect should not wait to read the entire content
+      response.disconnect();
+    }
+  }
 }