Adapter support for Net::HTTP::Persistent v3.0.0 #619

olleolleolle · 2016-10-14T11:44:03Z

This PR attempts to support Net::HTTP::Persistent 3.x or 2.x with its existing Faraday Adapter.

Changes introduced

For Net::HTTP::Persistent 3.x, there were changed APIs which needed changes in the existing Faraday Adapter
- a constructor had gotten keyword arguments instead of positional ones
- a cleaning-up method after SSL connections had been renamed
Introduce the use of ruby RUBY_VERSION Ruby version hinting for Bundler in the Gemfile.
- The Gemfile is used during development and during test runs in CI.
- with this, it was possible to avoid pinning to existing versions, and have the latest versions available supported by the currently running platform

TODO

use parameters instead of Net::HTTP::Persistent::VERSION to find out which version is running

Out of scope for this PR:

tests for all 2.x/3.x of Net::HTTP::Persistent on all Ruby platforms. Example solution: Thoughtbot's Appraisal gem with sets of generated Gemfiles

mislav

Hey, thanks for trying to solve this!

mislav · 2016-10-14T17:53:06Z

Gemfile

@@ -13,7 +13,11 @@ group :test do
  gem 'httpclient', '>= 2.2'
  gem 'mime-types', '~> 1.25', :platforms => [:jruby, :ruby_18]
  gem 'minitest', '>= 5.0.5'
-  gem 'net-http-persistent', '~> 2.9.4'
+  if RUBY_VERSION >= '2.1'


We can't use conditionals like these in a Gemfile. However, you can use the :platform to scope only to certain Rubies.

http://bundler.io/v1.13/man/gemfile.5.html#PLATFORMS seems not to support this use-case.

http://bundler.io/v1.13/man/gemfile.5.html#INSTALL_IF is perhaps a candidate?

[!] There was an error parsing `Gemfile`: You cannot specify the same gem twice with different version requirements. You specified: net-http-persistent (>= 0) and net-http-persistent (~> 2.9.4). Bundler cannot continue. # from /Users/olle/opensource/faraday/Gemfile:20 # ------------------------------------------- # install_if -> { RUBY_VERSION < '2.1' } do > gem 'net-http-persistent', '~> 2.9.4' # end # -------------------------------------------

@mislav This looks like the only working solution and is what it was done already in the past for other gems at the bottom of the Gemfile. Given the Gemfile is only used for tests, I think that is acceptable.

@iMacTia Would you want a squash-and-rebase? Any other changes?

No, I personally think that even though it's not the most elegant solution, this is the only one we can use in this specific case. So I'm saying that for me it's ok as it is.

mislav · 2016-10-14T17:55:25Z

lib/faraday/adapter/net_http_persistent.rb

@@ -17,8 +17,11 @@ def with_net_http_connection(env)
            define_method(:password) { proxy[:password] }
          end if proxy[:user]
        end
-
-        yield Net::HTTP::Persistent.new 'Faraday', proxy_uri
+        if Net::HTTP::Persistent::VERSION >= '3'


Instead of checking a version string like this, could we simply check the arity of the constructor?

Net::HTTP::Persistent.method(:new).arity

We could detect how many arguments does the constructor expect this way, and switch between two different invocation styles without being dependent on the version.

Our problem here is that they both (3.x and 2.x) return -1 for that arity - they both default on each of the given arguments.

Inspired by this comment, I changed the test invocation to avoid testing for the version.

🙇🏻

I switched this to open the possibility of adding arbitrary configuration keyword argument to the constructor. Keeping the few positional arguments seemed silly.

This will break if there is ever a net-http-persistent 10 release, but otherwise is fine.

mislav · 2016-10-14T17:55:48Z

test/adapters/net_http_persistent_test.rb

@@ -10,8 +10,10 @@ def setup
        if defined?(Net::HTTP::Persistent)
          # work around problems with mixed SSL certificates
          # https://github.com/drbrain/net-http-persistent/issues/45
-          http = Net::HTTP::Persistent.new('Faraday')
-          http.ssl_cleanup(4)
+          if Net::HTTP::Persistent::VERSION < '3'


Ditto my comment about version string checking.

iMacTia · 2016-10-21T12:47:11Z

test/adapters/net_http_persistent_test.rb

+          if Net::HTTP::Persistent::instance_methods.include?(:ssl_cleanup)
+            http = Net::HTTP::Persistent.new('Faraday')
+            http.ssl_cleanup(4)
+          end


Was this fixed in version 3.0 or simply the method was removed/renamed?
The issue appears to be still open and we're now skipping the ssl_cleanup...

@iMacTia: There's no longer any "cleanup" method in 3.0.0.

https://github.com/drbrain/net-http-persistent/commits/master

(Investigating.)

Is this the current cleanup? drbrain/net-http-persistent@dad5d68

I begin to think that this is the current way of "cleaning up" after finishing a connection.

Here's the ssl_cleanup implementation's comment (in v2.9.4):

Finishes all connections on the given +thread+ that were created before

the given +generation+ in the threads +generation_key+ list.

See #shutdown for a bunch of scary warning about misusing this method.

right, looks like you have to use this reuse_ssl_sessions. Did you spot also what the default value is?

This 2011 note from History.txt said:

For HTTPS connections, SSL sessions are now reused avoiding the extra
round trips and computations of extra SSL handshakes. If you have
problems with SSL session reuse it can be disabled by
Net::HTTP::Persistent#reuse_ssl_sessions

Answer: By default, SSL connections are reused: https://github.com/drbrain/net-http-persistent/blob/master/lib/net/http/persistent.rb#L409 Set to false to not do that.

Ok so in order for tests to work properly, we need to set this to false at the beginning.
I'm wondering if this option exists on v2.9 as well given it was there in 2011 😮

The comment immediately above this references issues with SSL client certificates, I think the correct thing to do is to call reconnect_ssl to address that.

I'll try adding a case for when that method is available and then use it.

But it is a test setup, and I'll have to check if the 4 in this cleanup isn't an exact ssl generation number to reset to (synthetically keeping it there to be able to count and compare ssl generation changing when different operations are tested).

Thanks for the hints, all!

olleolleolle · 2016-11-22T15:49:54Z

@drbrain Sorry I took forever, but I made a conditional else case with the v3.0.0 method you named. The tests still pass. Did I make it the way you mentioned?

olleolleolle · 2016-11-22T15:59:36Z

@iMacTia The last change to the Gemfile is one I was really aching to get in: using the ruby version marker to hint to Bundler's Rubygem-finding that we only need matching gems for our version.

See Bundler's docs on Gemfile for (spare) details: http://bundler.io/v1.13/man/gemfile.5.html#RUBY

I hope it can be used as it is; support for it arrived in Bundler 1.2 - otherwise we need to have a before_install: step in .travis.yml where we install latest Bundler.

iMacTia · 2016-11-22T16:18:41Z

Maybe I'm missing something, but looking at this I don't find a reason to specify something like ruby RUBY_VERSION in the Gemfile.
I understand using it to fi it to a specific version, but using RUBY_VERSION doesn't really make sense to me. Am I missing something?

drbrain · 2016-11-22T18:34:13Z

test/adapters/net_http_persistent_test.rb

@@ -10,8 +10,13 @@ def setup
        if defined?(Net::HTTP::Persistent)
          # work around problems with mixed SSL certificates
          # https://github.com/drbrain/net-http-persistent/issues/45
-          http = Net::HTTP::Persistent.new('Faraday')
-          http.ssl_cleanup(4)
+          if Net::HTTP::Persistent::instance_methods.include?(:ssl_cleanup)


#reconnect_ssl exists back to net-http-persistent v2.4.1, so you probably don't need this if

(Well, I needed the if in order to new with the 2 arities of constructor.)

olleolleolle · 2016-11-22T20:38:28Z

@iMacTia I understand using it to fi it to a specific version, but using RUBY_VERSION doesn't really make sense to me. Am I missing something?

Oh, the very thing is that we're not a deployed application where dependencies are to be locked down to 1 and only 1 gem selection, but a library which needs 1 selection per Ruby version we support.

Warning: This is more of a current measure, Bundler's future has better support for knowing this on its own.

So by leveraging Bundler's "Ruby version hinting", we can try to minimize ifs in the Gemfile.

Bundler outsources the decision about sub-dependencies' version needs to the gemspecs involved, by taking the required_ruby_version gemspec data into account.

(You know, I'll try improving the Bundler website with an FAQ item about just this.)

joevandyk · 2017-02-27T19:00:32Z

any update on this?

iMacTia · 2017-03-03T10:14:10Z

There are some important changes to the Gemfile, so I would like @mislav to review this PR as well before merging it in.

aaronmcadam · 2017-06-14T11:05:09Z

Is the issue about how to support both 2.x and 3.x of net-http-persistent?

olleolleolle · 2017-06-14T11:18:58Z

@aaronmcadam Yes!

aaronmcadam · 2017-06-14T11:20:42Z

Hey @olleolleolle I've not used it myself, but I've seen https://github.com/thoughtbot/appraisal being used for supporting multiple gem versions.

olleolleolle · 2017-06-14T11:22:56Z

@aaronmcadam I will extend the Description of this PR, in order to make it clear what the changes are and why they're needed.

JulienItard · 2017-06-19T10:27:40Z

Hi @olleolleolle ! Any idea when this will be available ?

Thanks 👍

iancward · 2017-06-29T14:59:49Z

@iMacTia @mislav any update on this? I just ran into this issue as well.

olleolleolle · 2017-06-29T15:03:20Z

@iancward To meet earlier requested change: Formulate a parameters-based version picking, instead of using the VERSION constant.

mtwentyman · 2017-07-18T14:51:17Z

If it is helpful, I've been using a version of the patch provided by @olleolleolle for a few projects (by way of making a new adapter available) and it has worked superbly.

iMacTia · 2017-07-19T17:42:37Z

Tests are passing and changes from @mislav have been implemented so it should now be mergeable.
I'll need to go through it one more time though and then I'll decide if including it into 0.12.2 (soon to be released) or 0.13.0 as it's mid-way between the feature and the fix type of PR 😅.
I know everyone is voting for 0.12.2 so no need to point that out 😄

I'm taking over the review :)

iMacTia · 2017-07-28T14:25:18Z

Tests are green and the Gemfile is cleaner now!
Moreover, Travis is testing both v2 and v3 of net_http_persistent as v2 will be installed for ruby < 2.1, v3 otherwise.
This was long and suffered, but thanks to @olleolleolle perseverance we can finally merge this in!
🎉 🎉 🎉 🎉 🎉

olleolleolle · 2017-07-28T15:21:38Z

Celebrate!

Reverts part of bb25acf The issue was resolved: lostisland/faraday#619

olleolleolle changed the title ~~Attempt to support Net::HTTP::Persistent v3.0.0~~ Adapter support for Net::HTTP::Persistent v3.0.0 Oct 14, 2016

mislav previously requested changes Oct 14, 2016

View reviewed changes

iMacTia reviewed Oct 21, 2016

View reviewed changes

iMacTia added feature unconfirmed labels Oct 21, 2016

iMacTia mentioned this pull request Nov 4, 2016

Faraday 0.9.2 and net-http-persistent 3.0.0 #617

Closed

iMacTia assigned mislav Nov 22, 2016

drbrain reviewed Nov 22, 2016

View reviewed changes

iMacTia mentioned this pull request May 17, 2017

Fix compatibility with Net::HTTP::Persistent v3. #692

Closed

jmartin-sul mentioned this pull request Jul 19, 2017

Pin net-http-persistent gem to address incompatibility with faraday 0.12 sul-dlss-deprecated/sdr-preservation-core#66

Merged

cbeer mentioned this pull request Jul 20, 2017

Constrain net-http-persistent to < 3.0 sul-dlss-deprecated/dor-workflow-client#44

Closed

iMacTia added this to the v0.13.0 milestone Jul 24, 2017

Adapter support for Net::HTTP::Persistent v3.0.0

163a024

iMacTia mentioned this pull request Jul 28, 2017

fix net_http_persistent adapter to be ready for >= v3.0.0 #716

Closed

iMacTia approved these changes Jul 28, 2017

View reviewed changes

iMacTia merged commit f8892b1 into lostisland:master Jul 28, 2017

danilotsr pushed a commit to remind101/faraday that referenced this pull request Aug 7, 2017

Adapter support for Net::HTTP::Persistent v3.0.0 (lostisland#619)

534def2

This was referenced Nov 16, 2018

Stripe::Subscription.retrieve and Stripe::Event.retrieve won't accept any arguments after upgrade to 4.0.1 stripe/stripe-ruby#700

Closed

Bump Faraday minimum version to 0.13.0 stripe/stripe-ruby#701

Merged

jcoyne added a commit to sul-dlss/common-accessioning that referenced this pull request Nov 30, 2018

Upgrade to net-http-persistent 3

85d7148

Reverts part of bb25acf The issue was resolved: lostisland/faraday#619

jcoyne mentioned this pull request Nov 30, 2018

Upgrade to net-http-persistent 3 sul-dlss/common-accessioning#122

Merged

russell mentioned this pull request Jan 10, 2019

net-http-persistent 3.0 and faraday incompatibility drbrain/net-http-persistent#80

Closed

Adapter support for Net::HTTP::Persistent v3.0.0 #619

Adapter support for Net::HTTP::Persistent v3.0.0 #619

Uh oh!

Conversation

olleolleolle commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes introduced

TODO

Out of scope for this PR:

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olleolleolle Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Finishes all connections on the given +thread+ that were created before

the given +generation+ in the threads +generation_key+ list.

See #shutdown for a bunch of scary warning about misusing this method.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olleolleolle Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olleolleolle commented Nov 22, 2016

Uh oh!

olleolleolle commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iMacTia commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olleolleolle commented Nov 22, 2016

Uh oh!

joevandyk commented Feb 27, 2017

Uh oh!

iMacTia commented Mar 3, 2017

Uh oh!

aaronmcadam commented Jun 14, 2017

olleolleolle commented Oct 14, 2016 •

edited

Loading

olleolleolle Oct 21, 2016 •

edited

Loading

olleolleolle Oct 21, 2016 •

edited

Loading

olleolleolle commented Nov 22, 2016 •

edited

Loading

iMacTia commented Jul 28, 2017 •

edited

Loading