Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpResponse.disconnect() broken for ApacheHttpTransport #1303

Closed
rawler opened this issue Mar 2, 2021 · 4 comments · Fixed by #1315
Closed

HttpResponse.disconnect() broken for ApacheHttpTransport #1303

rawler opened this issue Mar 2, 2021 · 4 comments · Fixed by #1315
Assignees
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@rawler
Copy link

rawler commented Mar 2, 2021

When using Apache as the underlying transport, issuing response.getContent().close() blocks waiting for the entire response to finish. If the source is the content from a GCS blob, this can potentially be many GB, which defeats the intention of disconnect().

Calling HttpResponse.disconnect() before .getContent().close() falls on the problem that disconnect() starts by calling getContent().close() before calling LowLevelHttpHandler.disconnect(). The wrong order, compared to the intended way for ApacheHttpClient.

The net effect is that the application-code get's blocked until all the blob-content have been read and discarded.

@Neenu1995 Neenu1995 added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 2, 2021
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Mar 8, 2021
@chingor13 chingor13 self-assigned this Mar 10, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 15, 2021
…esponse InputStream (#1315)

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
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 15, 2021
🤖 I have created a release \*beep\* \*boop\*
---
### [1.39.1](https://www.github.com/googleapis/google-http-java-client/compare/v1.39.0...v1.39.1) (2021-03-15)


### Bug Fixes

* default application/json charset to utf-8 ([#1305](https://www.github.com/googleapis/google-http-java-client/issues/1305)) ([c4dfb48](https://www.github.com/googleapis/google-http-java-client/commit/c4dfb48cb8248564b19efdf1a4272eb6fafe3138)), closes [#1102](https://www.github.com/googleapis/google-http-java-client/issues/1102)
* when disconnecting, close the underlying connection before the response InputStream ([#1315](https://www.github.com/googleapis/google-http-java-client/issues/1315)) ([f84ed59](https://www.github.com/googleapis/google-http-java-client/commit/f84ed5964f376ada5eb724a3d1f3ac526d31d9c5)), closes [#1303](https://www.github.com/googleapis/google-http-java-client/issues/1303)


### Documentation

* 19.0.0 libraries-bom ([#1312](https://www.github.com/googleapis/google-http-java-client/issues/1312)) ([62be21b](https://www.github.com/googleapis/google-http-java-client/commit/62be21b84a5394455d828b0f97f9e53352b8aa18))
* update version ([#1296](https://www.github.com/googleapis/google-http-java-client/issues/1296)) ([f17755c](https://www.github.com/googleapis/google-http-java-client/commit/f17755cf5e8ccbf441131ebb13fe60028fb63850))


### Dependencies

* update dependency com.fasterxml.jackson.core:jackson-core to v2.12.2 ([#1309](https://www.github.com/googleapis/google-http-java-client/issues/1309)) ([aa7d703](https://www.github.com/googleapis/google-http-java-client/commit/aa7d703d94e5e34d849bc753cfe8bd332ff80443))
* update dependency com.google.protobuf:protobuf-java to v3.15.3 ([#1301](https://www.github.com/googleapis/google-http-java-client/issues/1301)) ([1db338b](https://www.github.com/googleapis/google-http-java-client/commit/1db338b8b98465e03e93013b40fd8d821ac245c8))
* update dependency com.google.protobuf:protobuf-java to v3.15.6 ([#1310](https://www.github.com/googleapis/google-http-java-client/issues/1310)) ([9cb50e4](https://www.github.com/googleapis/google-http-java-client/commit/9cb50e49e1cfc196b915465bb6ecbd90fb6d04d7))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@chanseokoh
Copy link
Contributor

chanseokoh commented Aug 12, 2021

This is breaking us. When we call httpResponse.disconnect(), this makes the connection closed first. Not knowing that, ContentLengthInputStream tries to consume and discard the remaining bytes in close(), and it throws an exception as read() returns -1 (end of stream) while they believe there are remaining bytes to consume.

 Caused by: org.apache.http.ConnectionClosedException: Premature end of Content-Length delimited message body (expected: 2; received: 0)
 	at org.apache.http.impl.io.ContentLengthInputStream.read(ContentLengthInputStream.java:178)
 	at org.apache.http.impl.io.ContentLengthInputStream.read(ContentLengthInputStream.java:198)
 	at org.apache.http.impl.io.ContentLengthInputStream.close(ContentLengthInputStream.java:101)
 	at org.apache.http.impl.execchain.ResponseEntityProxy.streamClosed(ResponseEntityProxy.java:142)
 	at org.apache.http.conn.EofSensorInputStream.checkClose(EofSensorInputStream.java:228)
 	at org.apache.http.conn.EofSensorInputStream.close(EofSensorInputStream.java:172)
 	at com.google.api.client.http.HttpResponse.ignore(HttpResponse.java:427)
 	at com.google.api.client.http.HttpResponse.disconnect(HttpResponse.java:441)

@chanseokoh
Copy link
Contributor

A user can consistently reproduce the above error just with

new ApacheHttpTransport().createRequestFactory()
    .reqFactory.buildGetRequest(new GenericUrl(url))
    .execute()
    .disconnect();

And if you look at the comments and thumbs-ups in the linked issue, there are a lot of users hitting this error.

I think until this error can be properly handled, we should revert the change in #1315.

@chanseokoh
Copy link
Contributor

Oh, actually, I see #1409 already open.

@chanseokoh
Copy link
Contributor

FTR, the early change has been reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
5 participants