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

Connection sharing #121

Closed
sferik opened this issue Apr 5, 2014 · 20 comments
Closed

Connection sharing #121

sferik opened this issue Apr 5, 2014 · 20 comments
Labels
Milestone

Comments

@sferik
Copy link
Contributor

sferik commented Apr 5, 2014

Now that v0.6.0 has shipped, I’ve started benchmarking the http branch of the twitter gem against master, which uses Faraday with Net::HTTP.

While the spec suite runs about 4X faster in the http branch, the following real-world example—which outputs the screen names of my Twitter friends that start and end with the letter “a”—performs about 2X slower (5 seconds vs. 10 seconds).

require 'twitter'

client = Twitter::REST::Client.new(consumer_key: 'abc', consumer_secret: '123')
client.friend_ids('sferik').each_slice(100) do |slice|
  client.users(slice).select { |u| u.screen_name.match(/^a\w*a$/i) }.each do |user|
    puts user.screen_name
  end
end

When I run:

ruby-prof examples/aa_friends.rb --mode=wall --sort=self --min_percent=1

It produces the following output:

 %self      total      self      wait     child     calls  name
 60.58     10.089    10.089     0.000     0.000      189   OpenSSL::SSL::SSLSocket#sysread 
 25.86      4.306     4.306     0.000     0.000        6   OpenSSL::SSL::SSLSocket#connect 
 11.91      1.983     1.983     0.000     0.000        6   TCPSocket#initialize 

As you can see, I’m initializing a new TCP socket and calling connect for each HTTP request, which takes about a second per request. In the master branch, I’m able to share a single HTTP connection across multiple request, which explains why it’s about 5 seconds faster.

How can I share a single connection across multiple HTTP requests with the HTTP gem? Without this feature, I won’t be able to replace faraday with http in the twitter gem. 😦

@sferik sferik added this to the v0.7 milestone Apr 5, 2014
@ixti
Copy link
Member

ixti commented Apr 5, 2014

This depends on #72 and adding of Connection: Keep-Alive support.

@tarcieri
Copy link
Member

tarcieri commented Apr 5, 2014

I had planned to add connection pools ala Net::HTTP::Persistent as well.

@sferik is this the sort of thing you're after?

https://github.com/drbrain/net-http-persistent

@ghost
Copy link

ghost commented Apr 5, 2014

not "needed" atm but having something like net-http-persistent in http would give me one more reason to not depend on net/http if i needed to re-use the same socket.

@sferik
Copy link
Contributor Author

sferik commented Apr 6, 2014

@tarcieri Yes, I’m looking for something functionally similar to net-http-persistent, however, I’m not such a fan of the interface, which reminds me of Net::HTTP. Have you thought about what the HTTP gem’s interface for persistent connections might look like?

@ghost
Copy link

ghost commented Apr 6, 2014

imho something like HTTP.persistent("https://www.google.com").get("/foo").get("/bar") could be nice.

@sferik
Copy link
Contributor Author

sferik commented Apr 6, 2014

@johnny5- I need to be able to make separate, unchained requests using the same connection object, so this would need to work:

connection = HTTP.persistent("https://www.google.com")
connection.get("/foo")
connection.get("/bar")

@ghost
Copy link

ghost commented Apr 6, 2014

there's no reason it couldn't work with my example and yours.

@ghost
Copy link

ghost commented Apr 6, 2014

i think it would mean response's would end up being stacked in connection though and im not sure if that's good.

@sferik
Copy link
Contributor Author

sferik commented Apr 6, 2014

@johnny5- I just wanted to make sure it doesn’t require chaining the requests in a single statement.

@ghost
Copy link

ghost commented Apr 6, 2014

supporting both might be odd, .get() could return a response that could proxy another .get() through to the same connection object, to implement chaining, but i think it's better if people more familiar with the code weigh in.

@ixti
Copy link
Member

ixti commented Apr 6, 2014

I'm totally opposed to idea of proxy objects. IMHO it's confusing magic. Why one would need to call get() without getting response? I imagine API to be something like this:

client = HTTP.persisted # => HTTP::Client::Persisted

users  = client.get("http://example.com/users.json")
groups = client.get("http://example.com/groups.json")

Persisted client will hold connection pool internally, so if one didn't flushed connection before making another one (above) it will create another connection. Once it will have more than one open (and fully flushed) connection to same host, it will close all superfluous ones. Also, if you will get different host - it will add another persisted connection to the pool. Persistence pool will be configurable (total size, per-host size, auto-flush upon next request).

Clarification on "if one didn't flushed connection before":

client = HTTP.persisted # => HTTP::Client::Persisted

# 0 open connections

puts client.get("http://example.com/users.json").to_s
puts client.get("http://example.com/users.json").to_s

# 1 open connection

resources = %w[users groups persmissions].map do |res|
  client.get "http://example.com/#{res}.json"
end

# 3 open connections

resources.each { |r| r.to_s }

# 1 open connection

client.get("http://foobar.com/users.json").to_s

# 2 open connections

@tarcieri
Copy link
Member

tarcieri commented Apr 6, 2014

My ideal API would be transparent... that each HTTP::Client would hold a pool of HTTP::Connection objects per origin (perhaps with a toggle to use a pool or not). I'd define an origin as a tuple of:

[(:http|:https), hostname, port]

@ghost
Copy link

ghost commented Apr 6, 2014

i totally agree a proxy on a response back to the original connection would be confusing as hell :)

@sferik
Copy link
Contributor Author

sferik commented Mar 27, 2015

Resolved via #184.

@sferik sferik closed this as completed Mar 27, 2015
@sferik sferik reopened this Mar 27, 2015
@sferik
Copy link
Contributor Author

sferik commented Mar 27, 2015

Hmmm…trying to move this issue into the v0.8.0 milestone but I’m getting an error. Maybe it’s related to the denial of service GitHub was facing earlier today? Anyway, this feature should be released in v0.8.0.

@sferik sferik closed this as completed Mar 27, 2015
@sferik
Copy link
Contributor Author

sferik commented Mar 27, 2015

@tarcieri @ixti Are you able to change the milestone to v0.8.0?

@tarcieri
Copy link
Member

@sferik GitHub is giving me an error when I try to do that o_O

@sferik
Copy link
Contributor Author

sferik commented Mar 27, 2015

@tarcieri Me too. I thought it might be a permissions thing. Let’s remember to try again when GitHub isn’t under attack. I’d guess they’ve temporarily disabled some non-critical services like reassigning milestones.

@ixti ixti removed this from the v1.0.0 milestone Mar 27, 2015
@ixti ixti modified the milestones: v0.8.0, v1.0.0 Mar 27, 2015
@ixti
Copy link
Member

ixti commented Mar 27, 2015

I found the workaround to assign milestones :D via issues index not from particular issue...

@sferik
Copy link
Contributor Author

sferik commented Mar 27, 2015

@ixti 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants