chunked encoding + GZIP prevents keep-alive

[eddavisson]

GZIPInputStream.read() starts returning -1 once it finishes reading the gzip trailer. Depending on the underlying InputStream, this may occur before it has finished consuming the underlying InputStream.

In particular, if the underlying InputStream is a ChunkedInputStream, the ChunkedInputStream never reads the final chunk header (which indicates a chunk of length 0).

When ChunkedInputStream.close() is called, it is in a non-STATE_DONE state, and does not call HttpClient.finished(), which is required for the HttpClient object to get into the keep alive cache.

One possible fix is here:
https://github.com/google/google-http-java-client/blob/acdaeacc3c43402d795266d2ebcb584abe918961/google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java#L363

Rather than returning a GZIPInputStream, return a proxy input stream that retains references to both the gzip stream and the underlying stream. The proxy's close() method first consumes the underlying stream by calling read() and then calls close() on the gzip stream.

[eddavisson]

It's possible this only applies for some HttpTransport types. For example, Apache's ChunkedInputStream.close() method indicates that it will consume the rest the stream automatically:
https://hc.apache.org/httpclient-3.x/apidocs/org/apache/commons/httpclient/ChunkedInputStream.html#close()

[wsh]

We've worked around this in our code for now, but it's definitely worth fixing in the underlying library. 😄

[elharo]

This is really weird. Is the issue that there's a GZIP file/blob in a much larger stream that contains extra data after the end of the gzipped part?

[eddavisson]

IIRC, the issue occurs when the GZIPed content is chunked, so there is a GZIPInputStream wrapping a ChunkedInputStream.

The GZIPInputStream reports that is has been exhausted as soon as it observes the GZIP trailer. However, the ChunkedInputStream is not exhausted until it observes the final chunk header (with length 0).

[eddavisson]

Note that the hack we put in place for google-cloud-datastore is going to become increasingly inconvenient in future JDK releases.

[hiranya911]

I believe the solution implemented in #840 is inadequate. Consider the following test case, which is very similar to the code used in the NetHttpTransport:

  @Test
  public void testConnectionReuse() throws Exception {
    for (int i = 0; i < 3; i++) {
      URL url = new URL("https://jsonplaceholder.typicode.com/todos/1");
      HttpURLConnection conn = (HttpURLConnection) url.openConnection();
      conn.setRequestMethod("GET");
      conn.addRequestProperty("accept-encoding", "gzip");

      InputStream original = conn.getInputStream();
      InputStream is = new GZIPInputStream(original);
      byte[] data = ByteStreams.toByteArray(is);
      System.out.println(new String(data));

      is.close();
    }
  }

I expect the above code to open only 1 connection due to keep-alive, but it actually opens 3:

$ sudo tcpdump -i eth0 'tcp[tcpflags] & (tcp-syn) != 0' and dst host typicode.com
14:34:01.425773 IP my.host.49644 > 104.28.28.194.https: Flags [S], seq 2833089104, win 64240, options [mss 1460,sackOK,TS val 2275669545 ecr 0,nop,wscale 7], length 0
14:34:01.548967 IP my.host.49646 > 104.28.28.194.https: Flags [S], seq 538857976, win 64240, options [mss 1460,sackOK,TS val 2275669668 ecr 0,nop,wscale 7], length 0
14:34:01.573442 IP my.host.49648 > 104.28.28.194.https: Flags [S], seq 3217697064, win 64240, options [mss 1460,sackOK,TS val 2275669693 ecr 0,nop,wscale 7], length 0

The solution in #840 simply wraps the GZIPInputStream in a ConsumingInputStream:

InputStream is = new ConsumingInputStream(new GZIPInputStream(original));

But this won't change the behavior, because ConsumingInputStream only does this:

@Override
  public void close() throws IOException {
    if (!closed && in != null) {
      try {
        ByteStreams.exhaust(this);
        super.in.close();
      } finally {
        this.closed = true;
      }
    }
  }

Since ByteStreams.exhaust() is simply trying to read any remaining bytes from the GZIPInputStream, it doesn't really help clean up the underlying the ChunkedInputStream.

To get the desired keep-alive behavior we need to do something like the following in the test case:

ByteStreams.exhaust(original);