Basic support for ConnectionPool and persistent connections

[marshall-lee]

This PR adds a connection pool options and also tells in README about a benefit of persistent connections. Not informing users about Net::HTTP weakness is just harmful, I think :)

[marshall-lee]

...When comparing Her to ActiveRecord it's the one of the important features Her lacks. And since creating connections is done by Her itself there is no other way to setup a pooling than add its support to Her.

connection_pool gem with its simple with { ... } semantic (acquire connection from pool for every request and release it immediately) is very suitable for stateless protocols like HTTP.

[marshall-lee]

You can read more about all this stuff in the updated README

[marshall-lee]

@hubert I don't think so. Using a connection pool is optional so installing a connection_pool gem is also optional. Her should not require it to work — marshall-lee@5288afd#diff-6b1740b8978bf165426acae9d1c17138R1

[marshall-lee]

add_runtime_dependency is an alias to add_dependency 😃

[marshall-lee]

I made it a development dependency only for running connection pool-related specs.

[marshall-lee]

Maybe it is worth to add a connection_pool as a dependency and use it every time. Even if there's only one connection, having a pool will automatically solve thread safety issues. However it can confuse existing Her users that have long running queries — they can get a Timeout::Error accidently.

[marshall-lee]

bump! /cc @hubert

[hubert]

@marshall-lee thanks as always for your contributions. this was actually on my list of things to enhance for her, so really glad that you are taking this one. have a few questions/comments.

1. could you create a new connection_pool_spec file that exercises the connection pool code? also, i'd like to see some specs that validate that the connection pool code is doing the right thing, in addition to just running without error. i know metaprogramming let's you really reduced the code you write, but it would be nice to have specs that document and exercise each of the methods that is generated.

2. i know Net::HTTP:Persistent maintains it's own connection pool. have you tested the code with this adapter to make sure they work nicely together?

3. could you move the additions to the README into the advanced section?i believe the top section should really be focused on getting the user up and running.

[marshall-lee]

@hubert

1. could you create a new connection_pool_spec

done!

2. i know Net::HTTP:Persistent maintains it's own connection pool. have you tested the code with this adapter to make sure they work nicely together?

It doesn't require a setup_pool at all and I mentioned it in README. However when using it with setup_pool nothing harmful will occur. There will be no concurrent workload from multiple threads on a single Net::HTTP::Persistent instance so its own pool just won't be used.

3. could you move the additions to the README into the advanced section

done!

[marshall-lee]

cc @hubert

[scottraio]

+1

[marshall-lee]

cc @edtjones @Foxpaul :)

[edtjones]

Thanks for the nudge! This looks really interesting. A few questions (I don't know the answers!): - Should we go one step further and use something with swappable backends like httparty? - with a connection pool the requests are usually blocking, so you can run out of connections. Obviously, disregarding the negative side effects of resending the same tcp packets in Net::HTTP, that wouldn't happen in the current implementation. Does that mean that this could break an existing implementation if someone naively added a connection pool which is too small?

…

-- Sent from my phone: please forgive the brevity. Ed Jones

On 27 Dec 2016, at 05:38, Vladimir Kochnev ***@***.***> wrote: cc @edtjones @Foxpaul :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[marshall-lee]

@edtjones Hi Ed! Sorry for late response.

Should we go one step further and use something with swappable backends like httparty?

Only if someone really-really needs it and wants to contribute :) But for me it would be ridiculous because Faraday itself is a solution for swappable backends. BTW supporting HTTParty doesn't make any sense because it's just a convenient wrapper around Net::HTTP but its benefits will be hidden from the user.

Nevertheless, making our own swappable backends solution won't become harder after adding a connection pool, it's implementation-agnostic.

with a connection pool the requests are usually blocking, so you can run out of connections. Obviously, disregarding the negative side effects of resending the same tcp packets in Net::HTTP, that wouldn't happen in the current implementation. Does that mean that this could break an existing implementation if someone naively added a connection pool which is too small?

Yes. Rule of thumb is to make a pool size equal to threads count.