drbrain / net-http-persistent

Thread-safe persistent connections with Net::HTTP
http://seattlerb.rubyforge.org/net-http-persistent
339 stars 117 forks source link

Connection pooling #50

Closed bhenderson closed 7 years ago

bhenderson commented 11 years ago

this is my first (public) attempt to solve connection pooling for #49

am I on the right track?

obviously I would add setting size and disabling.

drbrain commented 11 years ago

I think this is close, but I would design it a different way.

I would have the Pool be an instance containing a size that is initialized in Net::HTTP::Persistent#initialize.

The pool can be used to clean up the use of @ssl_generation_key and @generation_key. Instead of incrementing the values of these keys a new Pool can be created and the old one cleaned up (the current #cleanup method would move there)

The pool should track the request and timeout along with the request such as:

connection, requests, timeout = pool.checkout net_http_args

Thread.current[:nhp_connection_requests] = requests
Thread.current[:nhp_connection_timeout] = timeout

Also, see the inline comments I am about to write.

bhenderson commented 11 years ago

I changed it to an instance and also made it so that if size is nil, it will act like it did before with one connection per thread.

I'm having problems working through moving connection creation to Pool as it seems like I would have to move quite a few methods over and it seems weird to have Pool do most of the work that this class is supposed to do. But I also know you have something in your head and I just need to work on understanding it more.

notlaforge commented 9 years ago

Stupid question: why would connection pooling be added as a feature of net-http-persistent, and not built as a pool object of its own, with new, borrow, invalidate and return?

Spaceghost commented 9 years ago

Knowing this isn't my project, I'd ask that the connection pool be a separate gem that the README.md links to.

ryansch commented 9 years ago

Is https://github.com/mperham/connection_pool useful here?

drbrain commented 9 years ago

Yes, I submitted changes to connection_pool to make it useable in NHP but haven't finished that work yet.

bhaibel commented 8 years ago

Has this been abandoned? If so, would anyone mind me picking the work up?

ryansch commented 8 years ago

Honestly I spent some time trying to get that to work and ended up realizing that net-http-persistent can't get away from raising Timeout::Error because it relies on net-http.

I wrote https://github.com/outstand/faraday_persistent_excon instead and it's been working great in production for quite some time. I'd like to eventually remove the faraday specific pieces and get it merged to excon itself.

drbrain commented 8 years ago

What is the problem with raising Timeout::Error in net/http?

In modern Ruby the only place Timeout.timeout is used is during connect.

drbrain commented 8 years ago

Note that master has connection pooling enabled using the connection-pool gem, please try it out.

bhaibel commented 8 years ago

Thank you!

--Betsy

On Mon, Jul 11, 2016 at 6:06 PM, Eric Hodel notifications@github.com wrote:

Note that master has connection pooling enabled using the connection-pool gem, please try it out.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/drbrain/net-http-persistent/pull/50#issuecomment-231880424, or mute the thread https://github.com/notifications/unsubscribe/AASb3AR1UWdl4KgicxF6HROfnqc8kUlfks5qUr58gaJpZM4A2RBD .

ryansch commented 8 years ago

I've had nothing but headaches with Timeout.timeout in our threaded code. There's honestly no reason that net-http couldn't use socket timeouts instead of spawning a thread that might raise at some odd location.

That aside, I think it's pretty great that you were able to add connection-pool support!

bhenderson commented 7 years ago

closing. thanks.