Skip to content
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

Optionally reuse http.Client #248

Closed
wants to merge 1 commit into from
Closed

Conversation

paulmach
Copy link

This is my proposed fix to https://github.com/crowdmob/goamz/issues/247

Basically connections are not being reused between requests. This causes a few problems:

  1. It's inefficient to renegotiate the SSL connection for every request.
  2. Leave tcp connections in a TIME_WAIT as they've been closed on the client side, but the os is waiting to see if the server has any late packets before it officially closes client side port.

These issue come up in my use case where I would like to download S3 objects at 200+ qps. I run out of local tcp sockets to use for new connections since they are all in the TIME_WAIT state. This fix solved my problems.

This PR adds an ReuseHTTPClient parameter to the S3 struct and the http.Client will be created on first request. The main issue is that changes to the ConnectTimeout and ReadTimeout will have not effect if made after the first request. (maybe we should check if they've been changed on each request?)

I understand that people have tried to solve this problem before and would be interested in their insights as I'm not an expert on TCP connection reuse and weird states it can find itself in.

@alimoeeny
Copy link
Contributor

It maybe worth having a look at https://github.com/goamz/goamz/blob/master/aws/client.go [ this is not the crowdmob/goamz it is goamz/goamz ]

@paulmach paulmach closed this Apr 1, 2015
@paulmach paulmach deleted the s3client branch April 1, 2015 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants