From 61487b582033b27f5883fdd032fb22bb094b439d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 10 Mar 2021 16:43:11 -0800 Subject: [PATCH 1/5] test: add failing test for waiting on response when disconnecting --- .../apache/v2/ApacheHttpTransportTest.java | 81 ++++++++++++++++--- .../http/javanet/NetHttpTransportTest.java | 65 +++++++++++++++ 2 files changed, 133 insertions(+), 13 deletions(-) diff --git a/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java b/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java index 4b9d9b8d7..fa91a0019 100644 --- a/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java +++ b/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,51 @@ 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 { + 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(); + } + } } diff --git a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java index 338236e9b..930655c4f 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java +++ b/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,59 @@ 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 { + 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(); + } + } } From 0f3689acd5b024abdcb1eef5aa5ea12b889071ed Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 10 Mar 2021 16:48:33 -0800 Subject: [PATCH 2/5] fix: close the underlying connection before trying to close the response InputStream --- .../java/com/google/api/client/http/HttpResponse.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java b/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java index 37f4d7f11..94785f02f 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java +++ b/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). @@ -432,8 +432,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(); } /** From 522041f2e74725255d08a1d37953a0a82df8d11d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 10 Mar 2021 16:53:58 -0800 Subject: [PATCH 3/5] test: add comment to the test case to explain what it's doing --- .../api/client/http/apache/v2/ApacheHttpTransportTest.java | 2 ++ .../google/api/client/http/javanet/NetHttpTransportTest.java | 2 ++ 2 files changed, 4 insertions(+) diff --git a/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java b/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java index fa91a0019..a0349ad8d 100644 --- a/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java +++ b/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java @@ -268,6 +268,8 @@ private boolean isWindows() { @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 diff --git a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java index 930655c4f..835730793 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java @@ -195,6 +195,8 @@ public void close() { @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 From 90ff8045dd5f00de73d577d57e9b7acd517ddbb8 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 10 Mar 2021 17:02:07 -0800 Subject: [PATCH 4/5] chore: fix variable name casing --- .../main/java/com/google/api/client/http/HttpResponse.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java b/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java index 94785f02f..72d5a5f7a 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java +++ b/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 contentencoding = this.contentEncoding.trim().toLowerCase(Locale.ENGLISH); - if (CONTENT_ENCODING_GZIP.equals(contentencoding) - || CONTENT_ENCODING_XGZIP.equals(contentencoding)) { + 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). From 0849b3def5f428d10235f755b59853ccb0db053f Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 15 Mar 2021 10:10:25 -0700 Subject: [PATCH 5/5] fix: directly close the low level response rather than using the logic to wrap the InputStream --- .../java/com/google/api/client/http/HttpResponse.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java b/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java index 72d5a5f7a..68d8850c8 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java @@ -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(); } }