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

Option to close connection for each HTTP request #31

Merged
merged 1 commit into from
Jul 8, 2016

Conversation

aanm
Copy link
Contributor

@aanm aanm commented Jul 7, 2016

Signed-off-by: André Martins [email protected]

@aanm aanm force-pushed the fixing-leaking-connections branch from 7012083 to 8e370bd Compare July 7, 2016 23:41
@jeevatkm
Copy link
Member

jeevatkm commented Jul 7, 2016

@aanm Thanks for the PR.

Technically there is no leak as per golang core. Below note taken from golang godoc-

// For client requests, setting this field prevents re-use of
// TCP connections between requests to the same hosts, as if
// Transport.DisableKeepAlives were set.
Close bool

So there is no issue with go-resty library. I will be renaming the PR subject.


I would say it up to developer or user choose the behavior they need. My recommendation is to expose some sort setter method to opt-in if they want to close connection for each request and defaults to false (also same as golang core).

your thoughts?

@jeevatkm jeevatkm changed the title Fixed leaking connections in HTTPRequests Option to close connection for each HTTP request Jul 8, 2016
@aanm
Copy link
Contributor Author

aanm commented Jul 8, 2016

@jeevatkm True, on my use case all connection were for the same host but with multiple resty clients. Setting that option to true dropped the number of connections open by 70%. I'll think on something an make it as an option. Any ideas where it would be the best place to put it?

@jeevatkm
Copy link
Member

jeevatkm commented Jul 8, 2016

You have specific use case. No issues. Best place is to -

  • Create a field called closeConnection after mutex field
  • Add SetCloseConnection for client struct like
    • func (c *Client) SetCloseConnection(close bool) *Client
    • Please add nice godoc documentation, so user can understand the purpose
  • Initialize explicitly on client creation after mutex field like closeConnection: false
  • Also please add SetCloseConnection method to default.go like you did in previous PR

finally assign in it the request (you already pick spot for this PR, but small change):

r.RawRequest.Close = c.closeConnection

@jeevatkm
Copy link
Member

jeevatkm commented Jul 8, 2016

Test case please and watch out travis ci build.

@jeevatkm jeevatkm self-assigned this Jul 8, 2016
@jeevatkm jeevatkm added this to the v0.8 Milestone milestone Jul 8, 2016
@aanm aanm force-pushed the fixing-leaking-connections branch from 8e370bd to 84d1c0e Compare July 8, 2016 15:38
@codecov-io
Copy link

codecov-io commented Jul 8, 2016

Current coverage is 96.99%

Merging #31 into master will increase coverage by 0.02%

@@             master        #31   diff @@
==========================================
  Files             5          5          
  Lines           759        766     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            736        743     +7   
  Misses           13         13          
  Partials         10         10          

Powered by Codecov. Last updated by 73f3362...84d1c0e

@aanm
Copy link
Contributor Author

aanm commented Jul 8, 2016

@jeevatkm ping

@jeevatkm jeevatkm merged commit 2cc8417 into go-resty:master Jul 8, 2016
@aanm aanm deleted the fixing-leaking-connections branch July 8, 2016 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants