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

Current stateless design break Keep-Alive support #41

Closed
byroot opened this issue Jul 30, 2011 · 7 comments
Closed

Current stateless design break Keep-Alive support #41

byroot opened this issue Jul 30, 2011 · 7 comments
Labels

Comments

@byroot
Copy link

byroot commented Jul 30, 2011

Currently a new adapter is instantiated for each HTTP request. It mean that a new HTTPClient or Net::HTTP instance is created too, and it's bad because it prevent them to store TCP connections in order to support the HTTP 1.1 keep alive feature which is a huge performance gain when you have to make multiple request on the same server.

@rubiii
Copy link
Contributor

rubiii commented Dec 26, 2012

@rogerleite i'm very interested to hear your ideas about this. as far as i can see, the problem is that each adapter persists its http client instance and if that instance is reused, we would need to "reset" all options set by the previous request. for example, if the first request configures curb to use http basic auth and the second request doesn't use basic auth, this option would need to be reset somehow.

@rogerleite
Copy link
Member

Maybe request object would generate a "checksum" based on its attributes, and the Adapter manager use this "checksum" and adapter symbol as key of pool of adapters instances. Something like that ...

@rubiii
Copy link
Contributor

rubiii commented Dec 27, 2012

interesting approach. when i started looking into this back then, i had problems resetting authentication for httpclient and realized that resetting things might not be the best approach, because not all clients allow you to do so. and even if they do, that might change in any version and if we don't have extensive tests for this, we might keep state that the user expects to be "reset", which might result in difficult to debug problems.

ps. as far as i can see, faraday has the same problem.

@rubiii
Copy link
Contributor

rubiii commented Dec 27, 2012

also: technoweenie/faraday#167

@rogerleite
Copy link
Member

Yesterday, i started researching on net/http and httpclient sources, looking for how they reuse connection. It was disappointing. Thanks to your link, i found something on net-http-persistent source.

The only alternative that came to me is:

  # HTTPI::Request
  def connection_id
    connection_info = [url.scheme,
                       url.host, url.port,
                       url.user, url.password]
    if self.proxy
      connection_info.concat [proxy.host, proxy.port,
                              proxy.user, proxy.password]
    end
    connection_info = connection_info.compact
    connection_id = connection_info.join(":")
    Digest::MD5.hexdigest(connection_id)
  end
    request = HTTPI::Request.new
    request.url = "http://example.com"
    puts "-> #{request.connection_id}" # => "4292bcf4853fa1b20babcd3591c0d714"
    request.query = {:q => "query"}
    puts "-> #{request.connection_id}" # => "4292bcf4853fa1b20babcd3591c0d714"

    request.proxy = "http://proxy.com"
    puts "-> #{request.connection_id}" # => "457c5182d77587276e4c2594ac3183fb"

On requests with same connection_id, HTTPI can reuse adapter instance.

The big question that remains is:
Is it worth!? I didn't find an easy way to measure if adapters are really reusing connections, as everybody are expecting.

@rubiii
Copy link
Contributor

rubiii commented Dec 27, 2012

for net/http we would probably need to add net-http-persistent to support keep-alive. as far as i know, httpclient and curb both support this feature. em-http-request seems to do so as well.

but you're raising the right question i think. for me, this is not top priority right now, but if we're going to support this, then the benefits need to outweigh the downsides and we need to make sure that it works as advertised without introducing any weird side effects from not properly caching/resetting the clients.

@pcai
Copy link
Member

pcai commented Jul 6, 2024

I consider this closed unless someone explains how to proceed - please see #238. thanks

@pcai pcai closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants