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

Reuse http.Client across requests, and allow caller to provide its own. #298

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

tomclegg
Copy link
Contributor

Enables connection sharing with http keepalive, and allows callers to
use Go's other http client/transport features like request timeouts.

Enables connection sharing with http keepalive, and allows callers to
use Go's other http client/transport features like request timeouts.
@ahmetb
Copy link
Contributor

ahmetb commented Mar 24, 2016

@tomclegg if I'm not mistaken, there should not be any difference between &http.Client{} and http.DefaultClient. Do you have a use case that requires you to change the HTTP client?

@tomclegg
Copy link
Contributor Author

@ahmetalpbalkan relevant differences between &http.Client{} and http.DefaultClient are

  • there is just one http.DefaultClient, vs. a new &http.Client{} each time you reach that LOC
  • a caller can modify http.DefaultClient before it gets used

My main motivation is that I want to let Go's http library do connection pooling with http keepalive, which it does automatically if you use the same Client for multiple requests.

This happens to provide access to other stdlib features too, like setting a timeout for http requests. It also lets you stub the Azure service for testing, without resorting to changing the global http.DefaultTransport.

(Is there any reason the caller shouldn't be allowed to provide its own http.Client?)

@ahmetb
Copy link
Contributor

ahmetb commented Mar 24, 2016

@tomclegg my only concern was we have another pr #217 coming in and it should probably rebase on top of this change. LGTM.

@ahmetb ahmetb closed this Mar 24, 2016
@ahmetb ahmetb reopened this Mar 24, 2016
@ahmetb ahmetb merged commit 056283f into Azure:master Mar 24, 2016
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.

3 participants