Use a connection pool for faraday connections

[workmad3]

Uses a connection pool around faraday clients instead of assuming the faraday connection is shareable arbitrarily across threads (it most likely isn't).

If a connection is assigned directly, this gets wrapped in a connection pool with a size of 1. Otherwise, the connection pool will be created as part of the client setup.

[balvig]

Also thanks for this very easy-to-read PR 😄
Just to understand a bit more about the situation, what sort of issues where you running into without this?

[workmad3]

We haven't run into any issues without this yet (partly because all our apps are currently using forking app servers, not threaded app servers).

I was just looking through the Spyke code with an eye to potentially introduce the ability to use faraday's parallel request features (e.g. using the Typhoeus adapter) and the lack of pooling around the faraday connection raised some alarm bells for me. I also had some concerns around whether all of the faraday adapters would play well with forking web servers, and the introduction of a lazy connection pool seemed to make sense.

[sschepens]

Is there a reason why this pr has not been merged? Connection pooling is a necessity when using threaded webservers such as Puma.

[balvig]

Is there a reason why this pr has not been merged

@sschepens only reason is my limited knowledge of threaded webservers 😅

Maybe you could help me better understand? I'd love to understand how this becomes a problem with puma! 🙇

[sschepens]

@balvig the issue relies in Faraday actually, Faraday connections are not guaranteed to be thread safe, so, multiple issues can arise when using threaded webservers because all threads share the same Faraday connection with Spyke right now.

It's really a mystery what happens when two threads use the same Faraday connection, I believe Faraday does not provide syncronization, thus relying on the adapters, so different adapters behave differently when used concurrently.

I created a script that tests all adapters used concurrently.
Some adapters (net_http, net_http_persistent, typhoeus, excon, httpclient) work ok and apparently perform each request in a new connection while others (em_http, em_http_synchrony, patron) throw weird exceptions, that mostly come from trying to reuse the same connection concurrently for multiple requests.

Most gems implement some sort of connection pooling around faraday to work around this issue, for example active-rest-client implements Thread local connections

Cheers!

[balvig]

@workmad3, @sschepens, thanks for the contribution and the explaining! 🙇

I have tweaked the PR a bit in https://github.com/balvig/spyke/compare/connection-pool, @workmad3 would be very happy if you have a second to look it over! 🙏 (if you enable me to edit this PR I can also push here)

Will also be testing this out in a few apps before releasing. Thanks again, and sorry for the time it took!

[waynerobinson]

We're still using Her/Faraday and recently switched to Puma for all our servers and have started running into deadlock issues.

I'm surprised I haven't seen anything about Faraday and thread-safety before. I seem to had reached the opinion that Faraday was thread-safe (and maybe it is), but it's not actually creating connections per-thread (or connection pooling) and hence why we're probably having these deadlocking issues.

On that note, I also discovered https://github.com/Ben-M/faraday_connection_pool which seems to attempt to add connection pooling to Faraday itself via a proxy to Net::HTTP. It's a couple of years since an update, but it seems to be a reasonably straight-forward piece of code. I wonder if this is a better solution than adding connection pooling at a higher level.

Thank you for this PR, at the very least it has finally opened my eyes to a very annoying set of bugs we've been experiencing lately.

[balvig]

Interesting!

Thanks a lot for that info @waynerobinson, would be super appreciative if when/if you decide to try out the faraday_connection_pool, could let us know if it solves the problem, would definitely be nice to have this "just work" when using faraday, instead of every library having to implement their own connection pool 😅

[brandondrew]

@balvig Is Spyke currently safe to use with Puma without this PR?