Repository navigation
resumable uploads do not implement keep-alive correctly #3531
Description
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
yihanzhen commentedon Aug 6, 2018
This would require work in google-http-java-client. cc/ @chingor13
chingor13 commentedon Aug 17, 2018
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#setThrowExceptionOnExecuteErrorand 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.
srosenberg commentedon Aug 18, 2018
@chingor13 Makes sense; any chance this fix would make the next maintenance release? Thanks!
ajaaym commentedon Jan 15, 2019
@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?
srosenberg commentedon Jan 15, 2019
@ajaaym The aforementioned issue occurs when running on oracle's jre
1.8. Conceivably, the implementation (ofdisconnect) may have changed going from1.8to1.9?ajaaym commentedon Jan 15, 2019
@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.
ajaaym commentedon Jan 15, 2019
@srosenberg does your program takes more than 5 second between two request?
srosenberg commentedon Jan 15, 2019
@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
disconnectis to close an existing socket; I verified this vianetstatandtcpdumpat the time. I don't have a standalone repro. at this time but will put something together in the next day or two.ajaaym commentedon Jan 16, 2019
@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.
srosenberg commentedon Feb 5, 2019
@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
keepAliveTimeoutto ~5 minutes to reduce the number of hard disconnects.ajaaym commentedon Feb 5, 2019
@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.
ajaaym commentedon Aug 1, 2019
Closing this as ApacheHttpTransport should resolve this.
chore(deps): update dependency com.google.cloud:google-cloud-bigquery…
deps: update opentelemetry.version to v1.45.0 (#3531)
chore: support for UUID type (#3572)
chore(main): release 2.64.2-SNAPSHOT (#3531)