Issue 235: Leak in NetHttpResponse

https://codereview.appspot.com/11472043/

Author: peleyal

findbugs-exclude.xml

@@ -78,4 +78,9 @@
     <Bug pattern="BETA_CLASS_USAGE,BETA_METHOD_USAGE"/>
     <Class name="com.google.api.client.http.HttpRequest"/>
   </And>
+  <And>
+    <Bug pattern="MS_PKGPROTECT"/>
+    <!-- Field should be package protected -->
+    <Class name="com.google.api.client.testing.http.javanet.MockHttpURLConnection"/>
+  </And>
 </FindBugsFilter>

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

@@ -968,7 +968,7 @@ public HttpResponse execute() throws IOException {
           response = new HttpResponse(this, lowLevelHttpResponse);
           responseConstructed = true;
         } finally {
-          if (!responseConstructed) {
+          if (!responseConstructed && lowLevelHttpResponse.getContent() != null) {
             lowLevelHttpResponse.getContent().close();
           }
         }

google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpResponse.java

@@ -14,7 +14,6 @@
 
 package com.google.api.client.http.javanet;
 
-import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.LowLevelHttpResponse;
 
 import java.io.IOException;
@@ -57,11 +56,34 @@ public int getStatusCode() {
     return responseCode;
   }
 
+  /**
+   * {@inheritDoc}
+   *
+   * <p>
+   * Returns {@link HttpURLConnection#getInputStream} when it doesn't throw {@link IOException},
+   * otherwise it returns {@link HttpURLConnection#getErrorStream}.
+   * </p>
+   *
+   * <p>
+   * Upgrade warning: in prior version 1.15 {@link #getContent()} returned
+   * {@link HttpURLConnection#getInputStream} only when the status code was successful. Starting
+   * with version 1.16 it returns {@link HttpURLConnection#getInputStream} when it doesn't throw
+   * {@link IOException}, otherwise it returns {@link HttpURLConnection#getErrorStream}
+   * </p>
+   */
   @Override
   public InputStream getContent() throws IOException {
-    HttpURLConnection connection = this.connection;
-    return HttpStatusCodes.isSuccess(responseCode)
-        ? connection.getInputStream() : connection.getErrorStream();
+    try {
+      return connection.getInputStream();
+    } catch (IOException ioe) {
+      InputStream stream = connection.getErrorStream();
+      // if the stream is not null then the error was set on the error stream, otherwise we should
+      // just rethrow the I/O exception.
+      if (stream == null) {
+        throw ioe;
+      }
+      return stream;
+    }
   }
 
   @Override

google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java

@@ -17,8 +17,10 @@
 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;
 import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.URL;
@@ -47,6 +49,24 @@ public class MockHttpURLConnection extends HttpURLConnection {
    */
   private OutputStream outputStream = new ByteArrayOutputStream(0);
 
+  /**
+   * The input byte array which represents the content when the status code is less then
+   * {@code 400}.
+   */
+  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}.
+   */
+  public static final byte[] ERROR_BUF = new byte[5];
+
+  /** The input stream. */
+  private InputStream inputStream = new ByteArrayInputStream(INPUT_BUF);
+
+  /** The error stream. */
+  private InputStream errorStream = new ByteArrayInputStream(ERROR_BUF);
+
   /**
    * @param u the URL or {@code null} for none
    */
@@ -109,4 +129,17 @@ public MockHttpURLConnection setResponseCode(int responseCode) {
     this.responseCode = responseCode;
     return this;
   }
+
+  @Override
+  public InputStream getInputStream() throws IOException {
+    if (responseCode < 400) {
+      return inputStream;
+    }
+    throw new IOException();
+  }
+
+  @Override
+  public InputStream getErrorStream() {
+    return errorStream;
+  }
 }

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

@@ -37,4 +37,24 @@ public void subtestGetStatusCode(int expectedCode, int responseCode) throws IOEx
     assertEquals(expectedCode, new NetHttpResponse(
         new MockHttpURLConnection(null).setResponseCode(responseCode)).getStatusCode());
   }
+
+  public void testGetContent() throws IOException {
+    subtestGetContent(0);
+    subtestGetContent(200);
+    subtestGetContent(304);
+    subtestGetContent(307);
+    subtestGetContent(404);
+    subtestGetContent(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]);
+    if (responseCode < 400) {
+      assertEquals(MockHttpURLConnection.INPUT_BUF.length, bytes);
+    } else {
+      assertEquals(MockHttpURLConnection.ERROR_BUF.length, bytes);
+    }
+  }
 }