From 49112d2d69d17e96f5082b4cbc827ebd0e6e6f28 Mon Sep 17 00:00:00 2001 From: Adrien Tateno Date: Fri, 4 May 2018 06:19:08 -0500 Subject: [PATCH] NetHttpPersistent adapter better reuse of SSL connections (#793) --- lib/faraday/adapter/net_http.rb | 12 +++-- lib/faraday/adapter/net_http_persistent.rb | 33 +++++++----- test/adapters/net_http_persistent_test.rb | 59 +++++++++++++++++----- 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index cd57cc86..3193d8ac 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/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.set_default_paths - cert_store + @cert_store = OpenSSL::X509::Store.new + @cert_store.set_default_paths + @cert_store end def ssl_verify_mode(ssl) diff --git a/lib/faraday/adapter/net_http_persistent.rb b/lib/faraday/adapter/net_http_persistent.rb index 4d45ae41..1e7ca94f 100644 --- a/lib/faraday/adapter/net_http_persistent.rb +++ b/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 env[:url], proxy_uri do + cached_connection 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.cert_store = ssl_cert_store(ssl) + http_set(http, :verify_mode, ssl_verify_mode(ssl)) + http_set(http, :cert_store, ssl_cert_store(ssl)) - http.certificate = ssl[:client_cert] if ssl[:client_cert] - http.private_key = ssl[:client_key] if ssl[:client_key] - http.ca_file = ssl[:ca_file] if ssl[:ca_file] - http.ssl_version = ssl[:version] if ssl[:version] + http_set(http, :certificate, ssl[:client_cert]) if ssl[:client_cert] + http_set(http, :private_key, ssl[:client_key]) if ssl[:client_key] + http_set(http, :ca_file, ssl[:ca_file]) if ssl[:ca_file] + http_set(http, :ssl_version, ssl[:version]) if ssl[:version] end - def cached_connection(url, proxy_uri) - (@cached_connection ||= {})[[url.scheme, url.host, url.port, proxy_uri]] ||= yield + def http_set(http, attr, value) + if http.send(attr) != value + http.send("#{attr}=", value) + end + end + + def cached_connection + @cached_connection ||= yield end end end diff --git a/test/adapters/net_http_persistent_test.rb b/test/adapters/net_http_persistent_test.rb index fc9e27ac..0261a917 100644 --- a/test/adapters/net_http_persistent_test.rb +++ b/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