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

[chingor13]

Adds a test to the NetHttpTransport and ApacheHttpTransport to make sure we don't wait until all the content is read when disconnecting the response.

Fixes #1303

[elharo]

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

[chingor13]

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).

[elharo]

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.

[chingor13]

That seems reasonable, I'll try it out.

[chingor13]

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.

[chingor13]

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".

[elharo]

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.

[suztomo]

@chingor13 I need help in upgrading this google-http-client dependency for Beam. With this PR 1315, google-http-client does not seem to read the content of responses with error status codes. Is this intended behavior when you made this pull request?

Background: I'm trying to upgrade the Beam's dependencies (apache/beam#14527) and was updating assertions. To check the reason of the behavior change, I found this PR 1315 in google-http-client. Most of them are trivial (changing the number of invocation of getStatusCode, getContent and, getContentType), but I think certain error handling logic (throwing exceptions and logging messages to users) cannot work without reading the content of HTTP responses. Example assertion I'm stuck: https://github.com/apache/beam/pull/14527/files/7fe131c6256774134fc0a8503628c7add97b9984#r613669141

    when(response.getStatusCode()).thenReturn(403).thenReturn(200);
    // Google-http-client 1.39.1 and later does not read content stream of failed response
    when(response.getContent())
        .thenReturn(toStream(errorWithReasonAndStatus("rateLimitExceeded", 403)))
        .thenReturn(toStream(testTable));
...

    BigQueryServicesImpl.DatasetServiceImpl services =
        new BigQueryServicesImpl.DatasetServiceImpl(
            bigquery, null, PipelineOptionsFactory.create());
    Table ret =
        services.tryCreateTable(
            testTable, new RetryBoundedBackOff(3, BackOff.ZERO_BACKOFF), Sleeper.DEFAULT);
...
    // This logging does not work after upgrading google-http-client to 1.39.2 (from 1.38.1)
    expectedLogs.verifyInfo(
        "Quota limit reached when creating table project:dataset.table, "
            + "retrying up to 5.0 minutes");

The tryCreateTable method relies on ApiErrorExtractor to log errors appropriatey.

https://github.com/apache/beam/blob/4b7b74673b647c8d964b4877a8d66d47096acce4/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java#L626

        } catch (IOException e) {
          ApiErrorExtractor extractor = new ApiErrorExtractor();
          if (extractor.itemAlreadyExists(e)) {
            // The table already exists, nothing to return.
            return null;
          } else if (extractor.rateLimited(e)) {

I appreciate if you can help me to update the test case for the latest google-http-client (or fix google-http-client if this is not intended behavior).

[elharo]

http client is supposed to close the stream on a deliberate client invoked disconnect. not necessarily on an http error response. Is beam calling disconnect somewhere?

[elharo]

The problem might be this code in HttpRequest, in particular the call to disconnect when throwExceptionOnExecuteError is true.

    // throw an exception if unsuccessful response
    if (throwExceptionOnExecuteError && !response.isSuccessStatusCode()) {
      try {
        throw new HttpResponseException(response);
      } finally {
        response.disconnect();
      }
    }

However I can't yet prove this with a test.

[elharo]

Is the BigQuery Beam test trying to read the content from an exception?

[elharo]

Might want to look into extractor.rateLimited(e) to see what it's doing. It might be incomplete or making invalid assumptions.

[suztomo]

@chingor13 @elharo I now think the problem seems to lie in the mock setup in Beam. At least it has assumption that lowLevelHttpRequest.getStatusCode() is called only once per response. (Thank you for the idea of FakeServer!) It's not that getContent was called fewer times, but getStatusCode was called more than the mock setup assumed.

I'll update you more.

[ChristianCiach]

This PR is the cause of issue GoogleContainerTools/jib#3409. I don't know if this PR is broken by itself or if Jib somehow uses the google-http-java-client in the wrong way. I just want to bring this to your attention.