From 8ae9cef3d8ea548ff2c55825dc89e109ec908a47 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Fri, 13 Mar 2015 14:37:06 -0700 Subject: [PATCH] Patch NetHttpResponse to detect abrupt termination JDK's HttpUrlConnection implementation fails to notify clients when the connection is lost when reading a response, e.g., the server gets restarted. This changelist patches the NetHttpResponse to detect abrupt connection termination when a fixed content length is available. In the case where responses are chunk encoded, connection lost is well handled by JDK. Also extend MockHttpUrlConnection for more testing functionalities. --- .../client/http/javanet/NetHttpResponse.java | 77 ++++++++++++++++++- .../http/javanet/MockHttpURLConnection.java | 76 +++++++++++++++++- .../http/javanet/NetHttpResponseTest.java | 47 ++++++++++- .../http/javanet/NetHttpTransportTest.java | 55 +++++++++++++ .../javanet/MockHttpUrlConnectionTest.java | 74 ++++++++++++++++++ 5 files changed, 320 insertions(+), 9 deletions(-) create mode 100644 google-http-client/src/test/java/com/google/api/client/testing/http/javanet/MockHttpUrlConnectionTest.java diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java index 16d3c717f..4a5edc7ec 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java @@ -16,6 +16,7 @@ import com.google.api.client.http.LowLevelHttpResponse; +import java.io.FilterInputStream; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; @@ -70,14 +71,27 @@ public int getStatusCode() { * with version 1.17 it returns {@link HttpURLConnection#getInputStream} when it doesn't throw * {@link IOException}, otherwise it returns {@link HttpURLConnection#getErrorStream} *

+ * + *

+ * Upgrade warning: in versions prior to 1.20 {@link #getContent()} returned + * {@link HttpURLConnection#getInputStream()} or {@link HttpURLConnection#getErrorStream()}, both + * of which silently returned -1 for read() calls when the connection got closed in the middle + * of receiving a response. This is highly likely a bug from JDK's {@link HttpURLConnection}. + * Since version 1.20, the bytes read off the wire will be checked and an {@link IOException} will + * be thrown if the response is not fully delivered when the connection is closed by server for + * whatever reason, e.g., server restarts. Note though that this is a best-effort check: when the + * response is chunk encoded, we have to rely on the underlying HTTP library to behave correctly. + *

*/ @Override public InputStream getContent() throws IOException { + InputStream in = null; try { - return connection.getInputStream(); + in = connection.getInputStream(); } catch (IOException ioe) { - return connection.getErrorStream(); + in = connection.getErrorStream(); } + return in == null ? null : new SizeValidatingInputStream(in); } @Override @@ -131,4 +145,63 @@ public String getHeaderValue(int index) { public void disconnect() { connection.disconnect(); } + + /** + * A wrapper arround the base {@link InputStream} that validates EOF returned by the read calls. + * + * @since 1.20 + */ + private final class SizeValidatingInputStream extends FilterInputStream { + + private long bytesRead = 0; + + public SizeValidatingInputStream(InputStream in) { + super(in); + } + + /** + * java.io.InputStream#read(byte[], int, int) swallows IOException thrown from read() so we have + * to override it. + * @see "http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/java/io/InputStream.java#185" + */ + @Override + public int read(byte[] b, int off, int len) throws IOException { + int n = in.read(b, off, len); + if (n == -1) { + throwIfFalseEOF(); + } else { + bytesRead += n; + } + return n; + } + + @Override + public int read() throws IOException { + int n = in.read(); + if (n == -1) { + throwIfFalseEOF(); + } else { + bytesRead++; + } + return n; + } + + // Throws an IOException if gets an EOF in the middle of a response. + private void throwIfFalseEOF() throws IOException { + long contentLength = getContentLength(); + if (contentLength == -1) { + // If a Content-Length header is missing, there's nothing we can do. + return; + } + // According to RFC2616, message-body is prohibited in responses to certain requests, e.g., + // HEAD. Nevertheless an entity-header (possibly with non-zero Content-Length) may be present. + // Thus we exclude the case where bytesRead == 0. + // + // See http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4 for details. + if (bytesRead != 0 && bytesRead < contentLength) { + throw new IOException("Connection closed prematurely: bytesRead = " + bytesRead + + ", Content-Length = " + contentLength); + } + } + } } diff --git a/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java b/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java index 2425e4edf..b59648e51 100644 --- a/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java +++ b/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java @@ -17,7 +17,6 @@ import com.google.api.client.util.Beta; import com.google.api.client.util.Preconditions; -import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; @@ -25,6 +24,10 @@ import java.net.HttpURLConnection; import java.net.URL; import java.net.UnknownServiceException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; /** * {@link Beta}
@@ -52,20 +55,28 @@ public class MockHttpURLConnection extends HttpURLConnection { /** * The input byte array which represents the content when the status code is less then * {@code 400}. + * + * @deprecated As of 1.20. Use {@link #setInputStream(InputStream)} instead. */ + @Deprecated public static final byte[] INPUT_BUF = new byte[1]; /** * The error byte array which represents the content when the status code is greater or equal to * {@code 400}. + * + * @deprecated As of 1.20. Use {@link #setErrorStream(InputStream)} instead. */ + @Deprecated public static final byte[] ERROR_BUF = new byte[5]; /** The input stream. */ - private InputStream inputStream = new ByteArrayInputStream(INPUT_BUF); + private InputStream inputStream = null; /** The error stream. */ - private InputStream errorStream = new ByteArrayInputStream(ERROR_BUF); + private InputStream errorStream = null; + + private Map> headers = new LinkedHashMap>(); /** * @param u the URL or {@code null} for none @@ -130,6 +141,54 @@ public MockHttpURLConnection setResponseCode(int responseCode) { return this; } + /** + * Sets a custom response header. + * + * @since 1.20 + */ + public MockHttpURLConnection addHeader(String name, String value) { + Preconditions.checkNotNull(name); + Preconditions.checkNotNull(value); + if (headers.containsKey(name)) { + headers.get(name).add(value); + } else { + List values = new ArrayList(); + values.add(value); + headers.put(name, values); + } + return this; + } + + /** + * Sets the input stream. + * + *

To prevent incidental overwrite, only the first non-null assignment is honored. + * + * @since 1.20 + */ + public MockHttpURLConnection setInputStream(InputStream is) { + Preconditions.checkNotNull(is); + if (inputStream == null) { + inputStream = is; + } + return this; + } + + /** + * Sets the error stream. + * + *

To prevent incidental overwrite, only the first non-null assignment is honored. + * + * @since 1.20 + */ + public MockHttpURLConnection setErrorStream(InputStream is) { + Preconditions.checkNotNull(is); + if (errorStream == null) { + errorStream = is; + } + return this; + } + @Override public InputStream getInputStream() throws IOException { if (responseCode < 400) { @@ -142,4 +201,15 @@ public InputStream getInputStream() throws IOException { public InputStream getErrorStream() { return errorStream; } + + @Override + public Map> getHeaderFields() { + return headers; + } + + @Override + public String getHeaderField(String name) { + List values = headers.get(name); + return values == null ? null : values.get(0); + } } diff --git a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpResponseTest.java b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpResponseTest.java index b86dd80a8..39fc04dfe 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpResponseTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpResponseTest.java @@ -15,10 +15,14 @@ package com.google.api.client.http.javanet; import com.google.api.client.testing.http.javanet.MockHttpURLConnection; +import com.google.api.client.util.StringUtils; import junit.framework.TestCase; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.Charset; /** * Tests {@link NetHttpResponse}. @@ -27,6 +31,9 @@ */ public class NetHttpResponseTest extends TestCase { + private static final String VALID_RESPONSE = "This is a valid response."; + private static final String ERROR_RESPONSE = "This is an error response."; + public void testGetStatusCode() throws IOException { subtestGetStatusCode(0, -1); subtestGetStatusCode(200, 200); @@ -45,16 +52,48 @@ public void testGetContent() throws IOException { subtestGetContent(307); subtestGetContent(404); subtestGetContent(503); + + subtestGetContentWithShortRead(0); + subtestGetContentWithShortRead(200); + subtestGetContentWithShortRead(304); + subtestGetContentWithShortRead(307); + subtestGetContentWithShortRead(404); + subtestGetContentWithShortRead(503); } public void subtestGetContent(int responseCode) throws IOException { NetHttpResponse response = - new NetHttpResponse(new MockHttpURLConnection(null).setResponseCode(responseCode)); - int bytes = response.getContent().read(new byte[100]); + new NetHttpResponse(new MockHttpURLConnection(null).setResponseCode(responseCode) + .setInputStream(new ByteArrayInputStream(StringUtils.getBytesUtf8(VALID_RESPONSE))) + .setErrorStream(new ByteArrayInputStream(StringUtils.getBytesUtf8(ERROR_RESPONSE)))); + InputStream is = response.getContent(); + byte[] buf = new byte[100]; + int bytes = 0, n = 0; + while ((n = is.read(buf)) != -1) { + bytes += n; + } + if (responseCode < 400) { + assertEquals(VALID_RESPONSE, new String(buf, 0, bytes, Charset.forName("UTF-8"))); + } else { + assertEquals(ERROR_RESPONSE, new String(buf, 0, bytes, Charset.forName("UTF-8"))); + } + } + + public void subtestGetContentWithShortRead(int responseCode) throws IOException { + NetHttpResponse response = + new NetHttpResponse(new MockHttpURLConnection(null).setResponseCode(responseCode) + .setInputStream(new ByteArrayInputStream(StringUtils.getBytesUtf8(VALID_RESPONSE))) + .setErrorStream(new ByteArrayInputStream(StringUtils.getBytesUtf8(ERROR_RESPONSE)))); + InputStream is = response.getContent(); + byte[] buf = new byte[100]; + int bytes = 0, b = 0; + while ((b = is.read()) != -1) { + buf[bytes++] = (byte) b; + } if (responseCode < 400) { - assertEquals(MockHttpURLConnection.INPUT_BUF.length, bytes); + assertEquals(VALID_RESPONSE, new String(buf, 0, bytes, Charset.forName("UTF-8"))); } else { - assertEquals(MockHttpURLConnection.ERROR_BUF.length, bytes); + assertEquals(ERROR_RESPONSE, new String(buf, 0, bytes, Charset.forName("UTF-8"))); } } } 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 6acc3f074..d8af273eb 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 @@ -21,7 +21,9 @@ import junit.framework.TestCase; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; import java.net.HttpURLConnection; import java.net.URL; @@ -59,6 +61,7 @@ public void testExecute_mock() throws Exception { } } + public void testExecute_methodUnchanged() throws Exception { for (String method : METHODS) { HttpURLConnection connection = @@ -75,6 +78,58 @@ public void testExecute_methodUnchanged() throws Exception { } } + public void testAbruptTerminationIsNoticedWithContentLength() throws Exception { + String incompleteBody = "" + + "Fixed size body test.\r\n" + + "Incomplete response."; + byte[] buf = StringUtils.getBytesUtf8(incompleteBody); + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)) + .setResponseCode(200) + .addHeader("Content-Length", "205") + .setInputStream(new ByteArrayInputStream(buf)); + connection.setRequestMethod("GET"); + NetHttpRequest request = new NetHttpRequest(connection); + setContent(request, null, ""); + NetHttpResponse response = (NetHttpResponse) (request.execute()); + + InputStream in = response.getContent(); + boolean thrown = false; + try { + while (in.read() != -1) { + // This space is intentionally left blank. + } + } catch (IOException ioe) { + thrown = true; + } + assertTrue(thrown); + } + + public void testAbruptTerminationIsNoticedWithContentLengthWithReadToBuf() throws Exception { + String incompleteBody = "" + + "Fixed size body test.\r\n" + + "Incomplete response."; + byte[] buf = StringUtils.getBytesUtf8(incompleteBody); + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)) + .setResponseCode(200) + .addHeader("Content-Length", "205") + .setInputStream(new ByteArrayInputStream(buf)); + connection.setRequestMethod("GET"); + NetHttpRequest request = new NetHttpRequest(connection); + setContent(request, null, ""); + NetHttpResponse response = (NetHttpResponse) (request.execute()); + + InputStream in = response.getContent(); + boolean thrown = false; + try { + while (in.read(new byte[100]) != -1) { + // This space is intentionally left blank. + } + } catch (IOException ioe) { + thrown = true; + } + assertTrue(thrown); + } + private void setContent(NetHttpRequest request, String type, String value) throws Exception { byte[] bytes = StringUtils.getBytesUtf8(value); request.setStreamingContent(new ByteArrayStreamingContent(bytes)); diff --git a/google-http-client/src/test/java/com/google/api/client/testing/http/javanet/MockHttpUrlConnectionTest.java b/google-http-client/src/test/java/com/google/api/client/testing/http/javanet/MockHttpUrlConnectionTest.java new file mode 100644 index 000000000..707af7ea3 --- /dev/null +++ b/google-http-client/src/test/java/com/google/api/client/testing/http/javanet/MockHttpUrlConnectionTest.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2015 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.api.client.testing.http.javanet; + +import com.google.api.client.testing.http.HttpTesting; +import com.google.api.client.util.StringUtils; + +import junit.framework.TestCase; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +/** + * Tests {@link MockHttpURLConnection}. + */ +public class MockHttpUrlConnectionTest extends TestCase { + + private static final String RESPONSE_BODY = "body"; + private static final String HEADER_NAME = "Custom-Header"; + + public void testSetGetHeaders() throws IOException { + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)); + connection.addHeader(HEADER_NAME, "100"); + assertEquals("100", connection.getHeaderField(HEADER_NAME)); + } + + public void testSetGetMultipleHeaders() throws IOException { + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)); + List values = Arrays.asList("value1", "value2", "value3"); + for (String value : values) { + connection.addHeader(HEADER_NAME, value); + } + Map> headers = connection.getHeaderFields(); + assertEquals(3, headers.get(HEADER_NAME).size()); + for (int i = 0; i < 3; i++) { + assertEquals(values.get(i), headers.get(HEADER_NAME).get(i)); + } + } + + public void testGetNonExistingHeader() throws IOException { + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)); + assertNull(connection.getHeaderField(HEADER_NAME)); + } + + public void testSetInputStreamAndInputStreamImmutable() throws IOException { + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)); + connection.setInputStream(new ByteArrayInputStream(StringUtils.getBytesUtf8(RESPONSE_BODY))); + connection.setInputStream(new ByteArrayInputStream(StringUtils.getBytesUtf8("override"))); + byte[] buf = new byte[10]; + InputStream in = connection.getInputStream(); + int n = 0, bytes = 0; + while ((n = in.read(buf)) != -1) { + bytes += n; + } + assertEquals(RESPONSE_BODY, new String(buf, 0, bytes)); + } +}