NetHttpPersistent adapter better reuse of SSL connections (#793)

lib/faraday/adapter/net_http.rb

@@ -28,6 +28,11 @@ module Faraday
       NET_HTTP_EXCEPTIONS << OpenSSL::SSL::SSLError if defined?(OpenSSL)
       NET_HTTP_EXCEPTIONS << Net::OpenTimeout if defined?(Net::OpenTimeout)
 
+      def initialize(app = nil, opts = {}, &block)
+        @cert_store = nil
+        super(app, opts, &block)
+      end
+
       def call(env)
         super
         with_net_http_connection(env) do |http|
@@ -119,10 +124,11 @@ module Faraday
 
       def ssl_cert_store(ssl)
         return ssl[:cert_store] if ssl[:cert_store]
+        return @cert_store if @cert_store
         # Use the default cert store by default, i.e. system ca certs
-        cert_store = OpenSSL::X509::Store.new
+        @cert_store = OpenSSL::X509::Store.new
-        cert_store.set_default_paths
+        @cert_store.set_default_paths
-        cert_store
+        @cert_store
       end
 
       def ssl_verify_mode(ssl)

lib/faraday/adapter/net_http_persistent.rb

@@ -6,15 +6,16 @@ module Faraday
       private
 
       def net_http_connection(env)
-        proxy_uri = proxy_uri(env)
+        cached_connection do
-
-        cached_connection env[:url], proxy_uri do
           if Net::HTTP::Persistent.instance_method(:initialize).parameters.first == [:key, :name]
-            Net::HTTP::Persistent.new(name: 'Faraday', proxy: proxy_uri)
+            Net::HTTP::Persistent.new(name: 'Faraday')
           else
-            Net::HTTP::Persistent.new('Faraday', proxy_uri)
+            Net::HTTP::Persistent.new('Faraday')
           end
         end
+        proxy_uri = proxy_uri(env)
+        cached_connection.proxy = proxy_uri if cached_connection.proxy_uri != proxy_uri
+        cached_connection
       end
 
       def proxy_uri(env)
@@ -46,17 +47,23 @@ module Faraday
       end
 
       def configure_ssl(http, ssl)
-        http.verify_mode  = ssl_verify_mode(ssl)
+        http_set(http, :verify_mode, ssl_verify_mode(ssl))
-        http.cert_store   = ssl_cert_store(ssl)
+        http_set(http, :cert_store,  ssl_cert_store(ssl))
 
-        http.certificate  = ssl[:client_cert]  if ssl[:client_cert]
+        http_set(http, :certificate, ssl[:client_cert]) if ssl[:client_cert]
-        http.private_key  = ssl[:client_key]   if ssl[:client_key]
+        http_set(http, :private_key, ssl[:client_key])  if ssl[:client_key]
-        http.ca_file      = ssl[:ca_file]      if ssl[:ca_file]
+        http_set(http, :ca_file,     ssl[:ca_file])     if ssl[:ca_file]
-        http.ssl_version  = ssl[:version]      if ssl[:version]
+        http_set(http, :ssl_version, ssl[:version])     if ssl[:version]
       end
 
-      def cached_connection(url, proxy_uri)
+      def http_set(http, attr, value)
-        (@cached_connection ||= {})[[url.scheme, url.host, url.port, proxy_uri]] ||= yield
+        if http.send(attr) != value
+          http.send("#{attr}=", value)
+        end
+      end
+
+      def cached_connection
+        @cached_connection ||= yield
       end
     end
   end

test/adapters/net_http_persistent_test.rb

@@ -17,6 +17,51 @@ module Adapters
           end
         end
       end if ssl_mode?
+
+      def test_reuses_tcp_sockets
+        # Ensure that requests are not reused from previous tests
+        Thread.current.keys
+          .select { |key| key.to_s =~ /\Anet_http_persistent_Faraday_/ }
+          .each { |key| Thread.current[key] = nil }
+
+        sockets = []
+        tcp_socket_open_wrapped = Proc.new do |*args, &block|
+          socket = TCPSocket.__minitest_stub__open(*args, &block)
+          sockets << socket
+          socket
+        end
+
+        TCPSocket.stub :open, tcp_socket_open_wrapped do
+          conn = create_connection
+          conn.post("/echo", :foo => "bar")
+          conn.post("/echo", :foo => "baz")
+        end
+
+        assert_equal 1, sockets.count
+      end
+
+      def test_does_not_reuse_tcp_sockets_when_proxy_changes
+        # Ensure that requests are not reused from previous tests
+        Thread.current.keys
+          .select { |key| key.to_s =~ /\Anet_http_persistent_Faraday_/ }
+          .each { |key| Thread.current[key] = nil }
+
+        sockets = []
+        tcp_socket_open_wrapped = Proc.new do |*args, &block|
+          socket = TCPSocket.__minitest_stub__open(*args, &block)
+          sockets << socket
+          socket
+        end
+
+        TCPSocket.stub :open, tcp_socket_open_wrapped do
+          conn = create_connection
+          conn.post("/echo", :foo => "bar")
+          conn.proxy = URI(ENV["LIVE_PROXY"])
+          conn.post("/echo", :foo => "bar")
+        end
+
+        assert_equal 2, sockets.count
+      end
     end
 
     def test_custom_adapter_config
@@ -31,19 +76,5 @@ module Adapters
 
       assert_equal 123, http.idle_timeout
     end
-
-    def test_caches_connections
-      adapter = Faraday::Adapter::NetHttpPersistent.new
-      a = adapter.send(:net_http_connection, :url => URI('https://example.com:1234/foo'), :request => {})
-      b = adapter.send(:net_http_connection, :url => URI('https://example.com:1234/bar'), :request => {})
-      assert_equal a.object_id, b.object_id
-    end
-
-    def test_does_not_cache_connections_for_different_hosts
-      adapter = Faraday::Adapter::NetHttpPersistent.new
-      a = adapter.send(:net_http_connection, :url => URI('https://example.com:1234/foo'), :request => {})
-      b = adapter.send(:net_http_connection, :url => URI('https://example2.com:1234/bar'), :request => {})
-      refute_equal a.object_id, b.object_id
-    end
   end
 end