search

Azure / azure-sdk-for-go

This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at:
https://docs.microsoft.com/azure/developer/go/
MIT License
1.64k stars 841 forks source link

Connection Reuse issue for PUT operations #404

Closed bcc closed 7 years ago

bcc commented 8 years ago

The calls that make use of an HTTP PUT are not properly closing the body, which is preventing reuse of HTTPS connections. This badly hurts performance, and can cause ephemeral connection exhaustion on the client. This is most obvious if you call something like PutBlob, SetBlobProperties or CopyBlob a lot of times.

I've bodged around this for now by adding io.Copy(ioutil.Discard, resp.body) after the operations that have been a problem for me as in https://github.com/bcc/azure-sdk-for-go/commit/df611657ca0c940d8f1c7d79959e3afa7f6aff38

It strikes me that this isn't a great solution though - none of the PUT operations on blobs actually do anything with the response body, and a skim through the other non-blob functions suggests that might be universally true. If that's the case, it might be better to fix this in client.exec by clearing out the body if verb == PUT, but I'm not sure if that might be a mistake later on.

marstr commented 7 years ago

@salameer @jhendrixMSFT @mcardosos This needs to be re-triaged.

marstr commented 7 years ago

I've spent some time this morning investigating, and I can confirm that this is a real issue. The root of the issue can be seen here: godoc http.Response which says:

The default HTTP client's Transport does not attempt to reuse HTTP/1.0 or HTTP/1.1 TCP connections ("keep-alive") unless the Body is read to completion and is closed.

For the vast majority of the methods in Azure/azure-sdk-for-go/storage, we call resp.body.Close() but never read the body; thus we complete only half of the contract that would result in connection reuse.

As @bcc noted above, it would be easiest to fix this in the client, by universally reading the response body then closing. We could keep the same contract by creating a ReadCloser over a buffer that we had copied the body into. However, the more thorough fix is to dig through all Storage operations and ensure the Body is read before it is closed where appropriate. I much prefer the latter solution, and believe it is worth the extra cost. I'll start working on a PR to fix this issue today.

marstr commented 7 years ago

This issue has now been fixed in storage (as mentioned in the original issue) and a PR is out to fix it in Azure/go-autorest.

Azure/go-autorest#118

I'll close this issue out for now. If the issue you were experiencing wasn't resolved, feel free to re-open.

meirf commented 6 years ago

@bcc did this fix work for you? I seem to be running into the same issue with the latest code.

jhendrixMSFT commented 6 years ago

@meirf which version of the SDK are you using? The latest is v15.0.0. Can you also paste the APIs you're using please?

meirf commented 6 years ago

@jhendrixMSFT I created a separate issue https://github.com/Azure/azure-sdk-for-go/issues/1500 since I'm not sure if they're related.