-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
WriteChannel for GCS doesn't read response body, leaks connections #4107
Comments
Hi, IIUC, the client should be clearing out the bytes in the body after the request is made to stop leaking bytes. Is this correct? |
That's right. If you do response.getContent() and then at least close the
returned InputStream the bad behavior goes away.
In my hokey patch that I have locally I also call readallbytes on the input
stream and that may be required to fully and reliably drain the socket on
both the receiver and the sender?
…On Tue, Nov 27, 2018, 5:38 PM Frank Natividad ***@***.*** wrote:
Hi,
IIUC, the client should be clearing out the bytes in the body after the
request is made to stop leaking bytes. Is this correct?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4107 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAmwfyL7WKIzZ8ALFzhM0rs9fjP0YRVlks5uzczdgaJpZM4Y2RAw>
.
|
Thanks @bobveznat! Could you also share the code that you are able to reproduce with? |
Okay, I think I was able to reproduce a similar issue. When the body of the response isn't closed e.g. After closing the request contents my example code completes successfully without hanging. I tried the following code: public static class CustomHttpTransportFactory implements HttpTransportFactory {
private static final HttpTransportFactory INSTANCE = new CustomHttpTransportFactory();
@Override
public HttpTransport create() {
PoolingHttpClientConnectionManager manager = new PoolingHttpClientConnectionManager();
manager.setMaxTotal(1);
return new ApacheHttpTransport(HttpClients.createMinimal(manager));
}
}
public void connectionPooling(String bucketName, String blobName) throws StorageException, IOException {
TransportOptions transportOptions = HttpTransportOptions.newBuilder().setHttpTransportFactory(new CustomHttpTransportFactory()).build();
Storage storage = StorageOptions.newBuilder().setTransportOptions(transportOptions).build().getService();
// First try
WriteChannel writeChannel = storage.writer(BlobInfo.newBuilder("bucket-name", "testobject").build());
ByteBuffer byteBuffer = ByteBuffer.wrap("hello world".getBytes());
writeChannel.write(byteBuffer);
writeChannel.close();
// Second try (The following will hang out!)
writeChannel = storage.writer(BlobInfo.newBuilder("bucket-name", "testobject").build());
byteBuffer = ByteBuffer.wrap("hello world".getBytes());
writeChannel.write(byteBuffer);
writeChannel.close();
} The fix for my reproduction was to close the response at HttpStorageRpc#685, using the following: Update to:
@bobveznat can you confirm if this is what you're seeing? |
Yes, that's very, very similar to what we do. Thank you for such a quick turnaround here. |
The fix is working for us. I upgraded to the 1.55 version on maven and pulled out my hack and I'm happy to report that I don't see sockets hanging out with data in the receive buffer anymore. Thank you! |
Thanks for the update @bobveznat! |
Environment details
Steps to reproduce
At a high level we're trying to upload 100s of objects to GCS ~quickly. We want to do this using an Apache HTTP client because we need to use a custom resolver. That said, I think this is broken on both Apache and other http clients, it just may not manifest itself the same way.
Under the hood the client will try to find an available connection from the pool and fail. It thinks the one we used earlier is still in use.
If you run ss or netstat on the machine at this point you can see an established connection to GCS with ~165 bytes sitting in the socket's receive buffer.
The reason is that we don't read the body of the response. See here:
https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java#L685
The response in both success and exception cases is discarded without ever reading or closing the response.
HttpRequest's execute() method pretty clearly states we need to do something with the response:
https://developers.google.com/api-client-library/java/google-http-java-client/reference/1.20.0/com/google/api/client/http/HttpRequest#execute()
The text was updated successfully, but these errors were encountered: