Use Faraday's Net::HTTP::Persistent adapter
#698
Conversation
| @@ -27,7 +27,8 @@ | |||
| # we can print one error and fail fast so that it's more clear to the user how | |||
| # they should fix the problem. | |||
| begin | |||
| resp = Faraday.get("http://localhost:#{MOCK_PORT}/") | |||
| conn = Faraday::Connection.new("http://localhost:#{MOCK_PORT}") | |||
| resp = conn.get("/") | |||
There was a problem hiding this comment.
I had to change this because using the default connection at the beginning of the test run "locked" the middleware stack and prevented me from changing the adapter above. I guess it only worked before by virtue of the fact that all the middlewares we installed were default and thus already installed (so no changes to the middleware stack needed to be made).
| c.use Faraday::Request::UrlEncoded | ||
| c.use Faraday::Response::RaiseError | ||
| c.adapter Faraday.default_adapter | ||
| conn = Faraday.new do |builder| |
There was a problem hiding this comment.
Changed this variable name to builder to be more accurate and descriptive as to what's being received inside the block here.
|
Thanks @brandur-stripe! It looks like this is causing issues with Ruby 2.0 and JRuby. We should probably consider dropping support for Ruby 2.0 altogether as it reached EOL 2.5 years ago. For JRuby, I'm not sure what the best course of action is -- maybe check the Ruby engine at runtime and only enable the |
brandur-stripe
force-pushed
the
brandur-net-http-persistent
branch
3 times, most recently
from
dcc6b0e to
18a9af1
Compare
|
Thanks @ob-stripe! Agreed on both accounts. Looking at your usage dashboard, it looks like Ruby 2.0 accounts for only 1% of Ruby requests, and given that it's been EOLed for so long at this point, it makes sense to drop it. I've pulled it out of the matrix here. JRuby's been given an alternate path that enables the default Faraday adapter instead. We still pull in the ptal @ob-stripe |
|
LGTM except that you should also bump |
This changes the library's default connection over to use the adapter for `Net::HTTP::Persistent`, which is a connection pooling library for Ruby. In the long run, I think we should probably just drop Faraday ... the amount of value it's getting us is extremely tenuous and its API is difficult to work with. I hate to do it at this point though because technically people could be writing custom middleware for it.
brandur-stripe
force-pushed
the
brandur-net-http-persistent
branch
from
18a9af1 to
85013c9
Compare
|
In the PR for this feature in stripe-php (stripe/stripe-php#550), I added a configuration flag to disable persistent connections, just in case it causes issues for some users. Should we do the same here? |
Oops, totally forgot that property was in there. Fixed, and thanks! Good catch.
Cool, will do. (We may want to re-evaluate the major version policy at some point though — I feel like it's a little over-conservative in some of these cases. We'll be coming up on the 3 year anniversary of Ruby 2.0's EOL — which keep in mind is when is when it died definitively, it had been deprecated for years before that — in only another 3 months!).
So users still have an out by configuring their own Faraday client. If problems are widespread I think a new config might make sense, but I think this is probably enough for now. Thoughts? |
Ah, very good point. I'll rework the PHP PR to do something similar. |
Good call. Thanks for the review! |
|
Released as 4.0.0. |
This changes the library's default connection over to use the adapter
for
Net::HTTP::Persistent, which is a connection pooling library forRuby.
In the long run, I think we should probably just drop Faraday ... the
amount of value it's getting us is extremely tenuous and its API is
difficult to work with. I hate to do it at this point though because
technically people could be writing custom middleware for it.
r? @ob-stripe
cc @stripe/api-libraries