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

Added connection reuse #184

Merged
merged 1 commit into from
Mar 26, 2015
Merged

Added connection reuse #184

merged 1 commit into from
Mar 26, 2015

Conversation

zanker
Copy link
Contributor

@zanker zanker commented Mar 26, 2015

Adds a connection reuse API to http.rb, via HTTP.persistent("http://www.google.com").

client = HTTP.persistent("http://www.google.ocm")
client.get("http://www.google.com/foo")
client.get("http://www.google.com/bar")

Results in reusing a connection. I went with the redundant host being passed because it keeps the API consistent. I could make it just /foo or /bar calls too.

@zanker
Copy link
Contributor Author

zanker commented Mar 26, 2015

cc @tarcieri

@zanker
Copy link
Contributor Author

zanker commented Mar 26, 2015

Last bit of work in and it should go green now.

I've added as many state checks for things like recovering from errors that I can think of. Since body's are lazily loaded, I've added sequence IDs. If you make multiple requests, you need to either have read out a body before making the next one, or you can't use the body later without it erroring.

@ixti
Copy link
Member

ixti commented Mar 26, 2015

There was a discussion about connection re-use: #121
Just want to get attention that in future we would like to have connections pool. I think of API like:

# @param [Hash] opts
# @option opts [Array<#to_s>] :hosts ([]) Makes sure connections persist for
#   the given list of hosts only. Any host if empty.
# @option opts [#to_i] :pool_size (5) Amount of simultaneous open connections.
def persistent(opts = {}); end

@zanker
Copy link
Contributor Author

zanker commented Mar 26, 2015

My concern with using a connection pool is that it adds a lot of additional complexity and I'm not sure that call patterns are worth it.

The connection_pool gem works great, and I'm not sure how beneficial it is to really reuse a client across multiple hosts.

@tarcieri
Copy link
Member

Yeah, I like that this is doing the simple thing, and agree that if pools are needed, connection_pool can be used.

@ixti
Copy link
Member

ixti commented Mar 26, 2015

Then I must admit I was thinking about some absolutely different API :D
But I agree that this version is pretty neat :D
After all advanced pooling can be added later as part of core or as a separate extension gem.

@sferik
Copy link
Contributor

sferik commented Mar 26, 2015

As the person who initially requested this feature, I’m 👍 on this pull request. The API is essentially identical to what I proposed in #121. Nice work @zanker! Thank you for implementing this!

@zanker
Copy link
Contributor Author

zanker commented Mar 26, 2015

@sferik Hah no problem. We're replacing our use of Net::HTTP::Persistent and the only gems with proper conn reuse are either JRuby or libcurl based :(

@zanker
Copy link
Contributor Author

zanker commented Mar 26, 2015

Made a quick change. On any exception we reset the conn, rather than a specific subset of network related ones.

Mostly it's a safety measure to be sure we don't have mixed requests/response.

tarcieri added a commit that referenced this pull request Mar 26, 2015
@tarcieri tarcieri merged commit 4bf5a21 into httprb:master Mar 26, 2015
@ixti
Copy link
Member

ixti commented Mar 26, 2015

👍

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.

None yet

4 participants