Skip to content

Commit

Permalink
fix: when disconnecting, close the underlying connection before the r…
Browse files Browse the repository at this point in the history 
…esponse 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
  • Loading branch information
@chingor13
chingor13 authored Mar 15, 2021
1 parent 9cb50e4 commit f84ed59
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 20 deletions.
83 changes: 70 additions & 13 deletions ...apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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();
}
}
}
19 changes: 12 additions & 7 deletions google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -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();
}

/**
Expand Down
67 changes: 67 additions & 0 deletions ...le-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
Expand Down Expand Up @@ -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();
}
}
}

0 comments on commit f84ed59

Please sign in to comment.