Faraday::Adapter::NetHttpPersistent does not reuse requests for SSL with Net::HTTP::Persistent < 3.0.0

[katsuya94]

Basic Info

• Faraday Version: 1.15.0, 1.13.1, 1.12.2
• Net::HTTP::Persistent version: 2.9.4
• Ruby Version: 2.2.3

Issue description

Faraday will use new connections for every request with the NetHttpPersistent adapter.

This is because Faraday::Adapter::NetHttp#call will call configure_ssl for every request made https://github.com/lostisland/faraday/blob/master/lib/faraday/adapter/net_http.rb#L34.

Faraday::Adapter::NetHttpPersistent#configure_ssl calls multiple methods on the Net::HTTP::Persistent instance that internally call Net::HTTP::Persistent#reconnect_ssl https://github.com/lostisland/faraday/blob/v0.15.0/lib/faraday/adapter/net_http_persistent.rb#L49.

reconnect_ssl increments a counter called @ssl_generation which is used as a key for the hash of available connections https://github.com/drbrain/net-http-persistent/blob/v2.9.4/lib/net/http/persistent.rb#L604.

Because this counter is incremented for every request, connections cannot be reused.

Steps to reproduce

require "net/http/persistent"
require "faraday"

connection = Faraday.new("https://www.google.com") do |f|
  f.adapter :net_http_persistent
end

module Hack
  def request(*args)
    puts "request #{object_id}"
    super
  end

  def initialize(*args)
    puts "initialize #{object_id}"
    super
  end
end

module PersistentHack
  def reconnect_ssl(*args)
    puts "reconnect_ssl #{@ssl_generation}"
    super
  end
end

Net::HTTP.send(:prepend, Hack)
Net::HTTP::Persistent.send(:prepend, PersistentHack)

puts "request 1"
connection.get("")

puts "request 2"
connection.get("")

Output

request 1
reconnect_ssl 0
reconnect_ssl 1
initialize 7789240
request 7789240
request 2
reconnect_ssl 2
reconnect_ssl 3
initialize 7744280
request 7744280

If we force Net::HTTP::Persistent#reconnect_ssl to no-op instead of incrementing the counter instead we get

request 1
initialize 13027780
request 13027780
request 2
request 13027780

which clearly shows the connection being reused.

[iMacTia]

Hi @katsuya94 and thanks for raising this.
I understand the Net::HTTP::Persistent adapter is not the best one available in Faraday 😅.
We're recently done some work to improve the situation (see #778) but maybe that doesn't apply to versions < 3.0.0.

To be honest I don't have any deep knowledge of the library itself as I've never used it and I have the same issue with other libraries as well (e.g. EM-related ones).

For this reason, I've decided to isolate adapters into separate gems and have Faraday acting as a core library. This way, the Faraday core team doesn't have to maintain every single adapter available and the community can build whichever adapter they want to build without asking for permission.
I'm not talking about a short term plan though, I'm currently rewriting the tests so that they can be reused in external libraries as well, but that will take some time.

As you can imagine, I would just push this change to a later date when the Net::HTTP::Persistent adapter will be isolated into its own gem as I don't feel comfortable in making a major refactoring of the adapter with my insufficient knowledge.

If you can identify any quick-win changes that don't require major refactoring and don't break backwards compatibility, I'm happy to review a PR.
Will also leave the issue open for future reference.