WriteChannel for GCS doesn't read response body, leaks connections

[bobveznat]

Environment details

• OS: linux / centos7
• Java version: 1.9
• google-cloud-java version(s):
  • com.google.cloud:google-cloud-storage:1.52.0
  • com.google.cloud:google-cloud-core-http:1.52.0
  • org.apache.httpcomponents:httpclient:4.5.5

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.

1. Use a custom transport factory builder in order to use Apache HTTP Client
2. Allocate a PoolingHttpClientConnectionManager in your transport, set the pool size to 1
3. Instantiate the Storage service using this transport factory
4. Allocate a write channel
5. Write to it
6. Close it
7. Allocate another write channel
8. Try to write to it.

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

[frankyn]

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?

[bobveznat]

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

[frankyn]

Thanks @bobveznat! Could you also share the code that you are able to reproduce with?

[frankyn]

Okay, I think I was able to reproduce a similar issue. When the body of the response isn't closed e.g. response.getContent().close();, the subsequent request hangs. I see 747 bytes in Recv-Q when using netstat on a hung test.

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:

HttpResponse response = httpRequest.execute();
code = response.getStatusCode();
message = response.getStatusMessage();
response.getContent().close();

@bobveznat can you confirm if this is what you're seeing?

[bobveznat]

Yes, that's very, very similar to what we do. Thank you for such a quick turnaround here.

[bobveznat]

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!

[frankyn]

Thanks for the update @bobveznat!