Closing input stream in HttpResponse.disconnect and fixing NPE

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

 /*
  * Copyright (c) 2010 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
@@ skipping 14 matching lines @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.zip.GZIPInputStream;
 
 /**
  * HTTP response.
  *
  * <p>
+ * Callers should call {@link #disconnect} when the HTTP response object is no longer needed.
+ * However, {@link #disconnect} does not have to be called if the response stream is properly
+ * closed. Example usage:
+ * </p>
+ *
+ * <pre>
+   HttpResponse response = request.execute();
+   try {
+     // process the HTTP response object
+   } finally {
+     response.disconnect();
+   }
+ * </pre>
+ *
+ * <p>
  * Implementation is not thread-safe.
  * </p>
  *
  * @since 1.0
  * @author Yaniv Inbar
  */
 public final class HttpResponse {
 
   /** Content-Type parameter pattern. */
   private static final Pattern CONTENT_TYPE_PARAM_PATTERN =
       Pattern.compile(";\\s*(\\S[^=]*)=([^;]*[^;\\p{Space}])");
 
   /** HTTP response content or {@code null} before {@link #getContent()}. */
   private InputStream content;
 
   /** Content encoding or {@code null}. */
   private final String contentEncoding;
 
   /** Content type or {@code null} for none. */
   private final String contentType;
 
   /** HTTP headers. */
   private final HttpHeaders headers;
 
   /** Low-level HTTP response. */
-  private LowLevelHttpResponse response;
+  LowLevelHttpResponse response;
 
   /** Status code. */
   private final int statusCode;
 
   /** Status message or {@code null}. */
   private final String statusMessage;
 
   /** HTTP transport. */
   private final HttpTransport transport;
 
@@ skipping 24 matching lines @@
 
   /**
    * Determines whether logging should be enabled on this response.
    *
    * <p>
    * Defaults to {@link HttpRequest#isLoggingEnabled()}.
    * </p>
    */
   private boolean loggingEnabled;
 
+  /** Signals whether the content has been read from the input stream. */
+  private boolean contentRead;
+
   HttpResponse(HttpRequest request, LowLevelHttpResponse response) {
     this.request = request;
     transport = request.getTransport();
     headers = request.getResponseHeaders();
     contentLoggingLimit = request.getContentLoggingLimit();
     loggingEnabled = request.isLoggingEnabled();
     this.response = response;
     contentType = response.getContentType();
     contentEncoding = response.getContentEncoding();
     int code = response.getStatusCode();
@@ skipping 193 matching lines @@
   }
 
   /**
    * Returns the content of the HTTP response.
    * <p>
    * The result is cached, so subsequent calls will be fast.
    *
    * @return input stream content of the HTTP response or {@code null} for none
    * @throws IOException I/O exception
    */
   public InputStream getContent() throws IOException {

[yanivi, 2012/05/30 21:07:26]

would help to add JavaDoc explaining that the content must be closed in a
finally, and also that you don't need to disconnect if you close the content

[yanivi, 2012/05/30 21:07:26]

implementation of getContent has a similar problem as HttpRequest.execute().  If
any of the code inside throws an exception of some kind we won't actually close
the stream.  so perhaps we need a try-finally and call
lowLevelResponseContent.close() or response.disconnect()

[rmistry, 2012/05/31 12:25:38]

Done.

[rmistry, 2012/05/31 12:25:38]

Done.

-    LowLevelHttpResponse response = this.response;
-    if (response == null) {
-      return content;
-    }
-    InputStream content = this.response.getContent();
-    this.response = null;
-    if (content != null) {
-      // gzip encoding (wrap content with GZipInputStream)
-      String contentEncoding = this.contentEncoding;
-      if (contentEncoding != null && contentEncoding.contains("gzip")) {
-        content = new GZIPInputStream(content);
+    if (!contentRead) {
+      InputStream lowLevelResponseContent = this.response.getContent();
+      if (lowLevelResponseContent != null) {
+        // gzip encoding (wrap content with GZipInputStream)
+        String contentEncoding = this.contentEncoding;
+        if (contentEncoding != null && contentEncoding.contains("gzip")) {
+          lowLevelResponseContent = new GZIPInputStream(lowLevelResponseContent);
+        }
+        // logging (wrap content with LoggingInputStream)
+        Logger logger = HttpTransport.LOGGER;
+        if (loggingEnabled && logger.isLoggable(Level.CONFIG)) {
+          lowLevelResponseContent = new LoggingInputStream(
+              lowLevelResponseContent, logger, Level.CONFIG, contentLoggingLimit);
+        }
+        content = lowLevelResponseContent;
       }
-      // logging (wrap content with LoggingInputStream)
-      Logger logger = HttpTransport.LOGGER;
-      if (loggingEnabled && logger.isLoggable(Level.CONFIG)) {
-        content = new LoggingInputStream(content, logger, Level.CONFIG, contentLoggingLimit);
-      }
-      this.content = content;
+      contentRead = true;
     }
     return content;
   }
 
   /**
    * Writes the content of the HTTP response into the given destination output stream.
    *
    * <p>
    * Sample usage: <code>
     HttpRequest request =
@@ skipping 31 matching lines @@
    * 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();
     }
   }
 
   /**
-   * Disconnect using {@link LowLevelHttpResponse#disconnect()}.
+   * Close the HTTP response content and disconnect using {@link LowLevelHttpResponse#disconnect()}.
+   *
+   * <p>
+   * Upgrade warning: since version 1.10 {@link #disconnect} now closes the HTTP response content
+   * input stream. This was not done by this method prior to version 1.10.
+   * </p>
    *
    * @since 1.4
    */
   public void disconnect() throws IOException {
+    ignore();
     response.disconnect();
   }
 
   /**
    * Returns the HTTP response content parser to use for the content type of this HTTP response or
    * {@code null} for none.
    */
   public HttpParser getParser() {
     return request.getParser(contentType);
   }
@@ skipping 50 matching lines @@
       Matcher m = CONTENT_TYPE_PARAM_PATTERN.matcher(contentType);
       while (m.find()) {
         if ("charset".equalsIgnoreCase(m.group(1))) {
           return m.group(2);
         }
       }
     }
     return "ISO-8859-1";
   }
 }