Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from Mar 15, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
61487b5
test: add failing test for waiting on response when disconnecting
chingor13 Mar 11, 2021
0f3689a
fix: close the underlying connection before trying to close the respo…
chingor13 Mar 11, 2021
522041f
test: add comment to the test case to explain what it's doing
chingor13 Mar 11, 2021
90ff804
chore: fix variable name casing
chingor13 Mar 11, 2021
0849b3d
fix: directly close the low level response rather than using the logi…
chingor13 Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 70 additions & 13 deletions ...apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java
View file
Open in desktop
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();
}
}
}
10 changes: 6 additions & 4 deletions google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
View file
Open in desktop
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);
chingor13 marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to call this at all? I'd think disconnecting alone is enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the original intention for sure, but it looks like it's supposed to close the response's InputStream so you can't read from it anymore - a way of letting google-http-client cleaning up the connections and their resources. This class was designed before AutoClosable was available (and possibly even before Closable was available).

Copy link
Contributor

@elharo elharo Mar 12, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I look at this, the more off this seems. I think what we should do is:

      InputStream lowLevelResponseContent = this.response.getContent();
      lowLevelResponseContent.close();

ignore() actually reads and decodes the bytes remaining on the stream. That could be quite a long-running operation. The point of disconnecting is to throw those away and not waste time reading them.

If you see otherwise, let's discuss.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable, I'll try it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified ignore() to directly close the LowLevelHttpResponse's InputStream rather than doing the extra reading logic in getContent().

I still do need to swap the disconnect() and ignore() lines as the ApacheHttpResponse will still try to read the content from its input stream unless it's disconnected first.

Comment on lines -435 to +441
Copy link
Collaborator Author

@chingor13 chingor13 Mar 11, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sold this is the best solution -- the tests do pass however.

An alternative is to mark that this response is "disconnecting" and in getContent() don't read from the underlying response if we're "disconnecting".

}

/**
Expand Down
67 changes: 67 additions & 0 deletions ...le-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpTransportTest.java
View file
Open in desktop
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();
}
}
}