Skip to content

resumable uploads do not implement keep-alive correctly #3531

Description

@srosenberg

We have implemented a multi-threaded storage uploader which uses BlobWriteChannel per thread to upload a chunk of a local file. (Once all chunks are uploaded, compose operation completes the transfer.) Despite keep-alive being enabled, (http) connections are not re-used, requiring a rather expensive TLS handshake on every subsequent (file) upload.

The problem lies in the current implementation of resumable uploads. Each intermediate PUT request (corresponding to a chunk being uploaded), except the last one, will result in 308 response code (see [1]) and consequently HttpResponse.disconnect (see [2]).

Thus, the corresponding connection is closed, effectively requiring a new handshake for each intermediate chunk in the subsequent upload.

[1] https://cloud.google.com/storage/docs/json_api/v1/how-tos/resumable-upload
[2] https://github.com/google/google-http-java-client/blob/release-1.24.1/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java#L1074

Activity

  1. added
    type: feature request‘Nice-to-have’ improvement, new feature or different behavior or design.
    api: storageIssues related to the Cloud Storage API.
    priority: p2Moderately-important priority. Fix may not be included in next release.
    on Aug 6, 2018
  2. yihanzhen commented on Aug 6, 2018

    @yihanzhen
    Contributor

    This would require work in google-http-java-client. cc/ @chingor13

  3. chingor13 commented on Aug 17, 2018

    @chingor13
    Contributor

    It looks like we can handle this upstream in google-cloud-storage. We can disable the throwing of the exception on a non success code by using HttpRequest#setThrowExceptionOnExecuteError and handle the 308 response separately.

    We don't really want to add special handling for 308 responses in the http client because 308s mean something different to other services and that library is for generic http requests.

  4. srosenberg commented on Aug 18, 2018

    @srosenberg
    Author

    @chingor13 Makes sense; any chance this fix would make the next maintenance release? Thanks!

  5. removed
    priority: p2Moderately-important priority. Fix may not be included in next release.
    on Oct 9, 2018
  6. ajaaym commented on Jan 15, 2019

    @ajaaym
    Contributor

    @srosenberg disconnect here is not disconnecting the actual network connection in my test on sun jre 1.9. Can you please provide provide the repro and env detail on which you are experiencing this behavior?

  7. srosenberg commented on Jan 15, 2019

    @srosenberg
    Author

    @ajaaym The aforementioned issue occurs when running on oracle's jre 1.8. Conceivably, the implementation (of disconnect) may have changed going from 1.8 to 1.9?

  8. ajaaym commented on Jan 15, 2019

    @ajaaym
    Contributor

    @srosenberg just checked with 1.8, dont see disconnect actually closing underlying network connection. when disconnect is called http here is always null which doesnt result in closing underlying network connection.

  9. ajaaym commented on Jan 15, 2019

    @ajaaym
    Contributor

    @srosenberg does your program takes more than 5 second between two request?

  10. srosenberg commented on Jan 15, 2019

    @srosenberg
    Author

    @ajaaym Yes, this module is used for batched uploads into storage; each batch may take tens of seconds up to a small multiple of one minute (we throttle them). I am fairly certain the (side)effect of disconnect is to close an existing socket; I verified this via netstat and tcpdump at the time. I don't have a standalone repro. at this time but will put something together in the next day or two.

  11. ajaaym commented on Jan 16, 2019

    @ajaaym
    Contributor

    @srosenberg by default connection cache cleaner thread clears the connection idle for more than 5 second. so when you send new request after 5 second it starts all over again. I was able to reproduce this behavior (with and without calling disconnect). see here keepAliveTimeout is returned 0 which results in using LIFETIME constant which is 5000ms.

  12. srosenberg commented on Feb 5, 2019

    @srosenberg
    Author

    @ajaaym I don't see how to override the default behavior (in jdk). Any suggestions? We have background threads that periodically upload logs into storage; they are scheduled to run every 5 minutes and take ~3 minutes on average to finish, hence ~2 minutes of idling. We'd need to set keepAliveTimeout to ~5 minutes to reduce the number of hard disconnects.

  13. ajaaym commented on Feb 5, 2019

    @ajaaym
    Contributor

    @srosenberg there is no way to override this in jdk. Best bet here is to use ApacheHttpTransport.
    Apache client keeps the connection open indefinitely if Keep-Alive header is not present in the response. which is the case here. Link to document. So default configuration should work fine. In case you need to configure client, you can build the HttpClient using HttpClientBuilder and pass in to constructor of ApacheHttpTransport.

  14. ajaaym commented on Aug 1, 2019

    @ajaaym
    Contributor

    Closing this as ApacheHttpTransport should resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api: storageIssues related to the Cloud Storage API.type: feature request‘Nice-to-have’ improvement, new feature or different behavior or design.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions