diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index c16227512..0abd11c82 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -37,6 +37,24 @@ def inherited(subclass) end end + # This module marks an Adapter as supporting a cached HTTP connection. + module CacheConnection + def connection(env) + recfg = reconfigure_connection?(env) + conn = !recfg && @connection + conn ||= build_connection(env) + @connection = conn unless recfg + + return conn unless block_given? + + yield conn + end + + def cache_connection? + true + end + end + extend Parallelism self.supports_parallel = false @@ -60,6 +78,10 @@ def connection(env) yield conn end + def cache_connection? + false + end + def call(env) env.clear_body if env.needs_body? env.response = Response.new @@ -104,5 +126,14 @@ def request_timeout(type, options) open: :open_timeout, write: :write_timeout }.freeze + + def reconfigure_connection?(env) + return unless (req = env && env[:request]) + + CONNECTION_OPTIONS.any? { |k| req.key?(k) } + end + + CONNECTION_OPTIONS = %i[proxy bind timeout open_timeout read_timeout + write_timeout].freeze end end diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index 2ba264e3b..2a04293aa 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -6,6 +6,8 @@ class Adapter class Excon < Faraday::Adapter dependency 'excon' + include CacheConnection + def build_connection(env) opts = opts_from_env(env) ::Excon.new(env[:url].to_s, opts.merge(@connection_options)) @@ -52,20 +54,11 @@ def read_body(env) def opts_from_env(env) opts = {} - amend_opts_with_ssl!(opts, env[:ssl]) if needs_ssl_settings?(env) - - if (req = env[:request]) - amend_opts_with_timeouts!(opts, req) - amend_opts_with_proxy_settings!(opts, req) - end - + amend_opts_with_ssl!(opts, env[:ssl]) + amend_opts_for_request!(opts, env[:request]) opts end - def needs_ssl_settings?(env) - env[:url].scheme == 'https' && env[:ssl] - end - OPTS_KEYS = [ %i[client_cert client_cert], %i[client_key client_key], @@ -79,6 +72,8 @@ def needs_ssl_settings?(env) ].freeze def amend_opts_with_ssl!(opts, ssl) + return unless ssl + opts[:ssl_verify_peer] = !!ssl.fetch(:verify, true) # https://github.com/geemus/excon/issues/106 # https://github.com/jruby/jruby-ossl/issues/19 @@ -91,6 +86,13 @@ def amend_opts_with_ssl!(opts, ssl) end end + def amend_opts_for_request!(opts, req) + return unless req + + amend_opts_with_timeouts!(opts, req) + amend_opts_with_proxy_settings!(opts, req) + end + def amend_opts_with_timeouts!(opts, req) if (sec = request_timeout(:read, req)) opts[:read_timeout] = sec diff --git a/lib/faraday/adapter/httpclient.rb b/lib/faraday/adapter/httpclient.rb index 539c15aaa..42274f4c4 100644 --- a/lib/faraday/adapter/httpclient.rb +++ b/lib/faraday/adapter/httpclient.rb @@ -6,31 +6,15 @@ class Adapter class HTTPClient < Faraday::Adapter dependency 'httpclient' - def build_connection(env) - @client ||= ::HTTPClient.new.tap do |cli| - # enable compression - cli.transparent_gzip_decompression = true - end - - if (req = env[:request]) - if (proxy = req[:proxy]) - configure_proxy @client, proxy - end - - if (bind = req[:bind]) - configure_socket @client, bind - end - - configure_timeouts @client, req - end - - if env[:url].scheme == 'https' && (ssl = env[:ssl]) - configure_ssl @client, ssl - end + include CacheConnection - configure_client @client - - @client + def build_connection(env) + conn = ::HTTPClient.new + conn.transparent_gzip_decompression = true + configure_client(conn) + configure_for_request(conn, env[:request]) + configure_ssl(conn, env[:ssl]) + conn end def call(env) @@ -91,6 +75,8 @@ def configure_proxy(client, proxy) # @param ssl [Hash] def configure_ssl(client, ssl) + return unless ssl + ssl_config = client.ssl_config ssl_config.verify_mode = ssl_verify_mode(ssl) ssl_config.cert_store = ssl_cert_store(ssl) @@ -117,6 +103,20 @@ def configure_timeouts(client, req) client.receive_timeout = sec end + def configure_for_request(client, options) + return unless options + + if (proxy = options[:proxy]) + configure_proxy(client, proxy) + end + + if (bind = options[:bind]) + configure_socket(client, bind) + end + + configure_timeouts(client, options) + end + def configure_client(client) @config_block&.call(client) end diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index 191568dd4..32cfe1a01 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -45,6 +45,12 @@ def build_connection(env) if http.respond_to?(:use_ssl=) http.use_ssl = env[:url].scheme == 'https' end + + # Only set if Net::Http supports it, since Ruby 2.5. + http.max_retries = 0 if http.respond_to?(:max_retries=) + + @config_block&.call(http) + configure_ssl(http, env[:ssl]) configure_request(http, env[:request]) end @@ -167,6 +173,8 @@ def configure_ssl(http, ssl) end def configure_request(http, req) + return unless req + if (sec = request_timeout(:read, req)) http.read_timeout = sec end @@ -176,14 +184,9 @@ def configure_request(http, req) http.write_timeout = sec end - if (sec = request_timeout(:open, req)) - http.open_timeout = sec - end - - # Only set if Net::Http supports it, since Ruby 2.5. - http.max_retries = 0 if http.respond_to?(:max_retries=) + return unless (sec = request_timeout(:open, req)) - @config_block&.call(http) + http.open_timeout = sec end def ssl_cert_store(ssl) diff --git a/lib/faraday/adapter/net_http_persistent.rb b/lib/faraday/adapter/net_http_persistent.rb index 2465ad93c..24aaeee92 100644 --- a/lib/faraday/adapter/net_http_persistent.rb +++ b/lib/faraday/adapter/net_http_persistent.rb @@ -6,10 +6,12 @@ class Adapter class NetHttpPersistent < NetHttp dependency 'net/http/persistent' + include CacheConnection + private def net_http_connection(env) - @cached_connection ||= + conn = if Net::HTTP::Persistent.instance_method(:initialize) .parameters.first == %i[key name] options = { name: 'Faraday' } @@ -22,10 +24,8 @@ def net_http_connection(env) end proxy_uri = proxy_uri(env) - if @cached_connection.proxy_uri != proxy_uri - @cached_connection.proxy = proxy_uri - end - @cached_connection + conn.proxy = proxy_uri if conn.proxy_uri != proxy_uri + conn end def proxy_uri(env) diff --git a/lib/faraday/adapter/patron.rb b/lib/faraday/adapter/patron.rb index c6e2935fb..10948b213 100644 --- a/lib/faraday/adapter/patron.rb +++ b/lib/faraday/adapter/patron.rb @@ -6,18 +6,14 @@ class Adapter class Patron < Faraday::Adapter dependency 'patron' + include CacheConnection + def build_connection(env) session = ::Patron::Session.new @config_block&.call(session) - if (env[:url].scheme == 'https') && env[:ssl] - configure_ssl(session, env[:ssl]) - end - - if (req = env[:request]) - configure_timeouts(session, req) - configure_proxy(session, req[:proxy]) - end - + configure_ssl(session, env[:ssl]) if env[:ssl] + configure_for_request(session, env[:connection]) + configure_for_request(session, env[:request]) session end @@ -86,9 +82,14 @@ def configure_ssl(session, ssl) end end - def configure_timeouts(session, req) + def configure_for_request(session, req) return unless req + configure_timeouts(session, req) + configure_proxy(session, req[:proxy]) + end + + def configure_timeouts(session, req) if (sec = request_timeout(:read, req)) session.timeout = sec end diff --git a/lib/faraday/options/connection_options.rb b/lib/faraday/options/connection_options.rb index 5a729406c..e9e63313b 100644 --- a/lib/faraday/options/connection_options.rb +++ b/lib/faraday/options/connection_options.rb @@ -5,7 +5,8 @@ module Faraday # connection object. class ConnectionOptions < Options.new(:request, :proxy, :ssl, :builder, :url, :parallel_manager, :params, :headers, - :builder_class) + :builder_class, :bind, :read_timeout, + :timeout, :open_timeout, :write_timeout) options request: RequestOptions, ssl: SSLOptions diff --git a/lib/faraday/options/env.rb b/lib/faraday/options/env.rb index d7dac7178..7bef9897e 100644 --- a/lib/faraday/options/env.rb +++ b/lib/faraday/options/env.rb @@ -46,10 +46,14 @@ module Faraday # # @!attribute reason_phrase # @return [String] + # + # @!attribute connection + # @return [Faraday::ConnectionOptions] The connection options for the + # current adapter. class Env < Options.new(:method, :request_body, :url, :request, :request_headers, :ssl, :parallel_manager, :params, :response, :response_headers, :status, - :reason_phrase, :response_body) + :reason_phrase, :response_body, :connection) # rubocop:disable Naming/ConstantName ContentLength = 'Content-Length' @@ -61,7 +65,7 @@ class Env < Options.new(:method, :request_body, :url, :request, # these requests, the Content-Length header is set to 0. MethodsWithBodies = Set.new(Faraday::METHODS_WITH_BODY.map(&:to_sym)) - options request: RequestOptions, + options request: RequestOptions, connection: ConnectionOptions, request_headers: Utils::Headers, response_headers: Utils::Headers extend Forwardable diff --git a/lib/faraday/rack_builder.rb b/lib/faraday/rack_builder.rb index 8d5040248..68f762cb6 100644 --- a/lib/faraday/rack_builder.rb +++ b/lib/faraday/rack_builder.rb @@ -202,14 +202,7 @@ def dup # :password - Proxy server password # :ssl - Hash of options for configuring SSL requests. def build_env(connection, request) - exclusive_url = connection.build_exclusive_url( - request.path, request.params, - request.options.params_encoder - ) - - Env.new(request.method, request.body, exclusive_url, - request.options, request.headers, connection.ssl, - connection.parallel_manager) + request.to_env(connection) end private diff --git a/lib/faraday/request.rb b/lib/faraday/request.rb index 352ce6cec..3ee289517 100644 --- a/lib/faraday/request.rb +++ b/lib/faraday/request.rb @@ -139,8 +139,12 @@ def marshal_load(serialised) # @return [Env] the Env for this Request def to_env(connection) - Env.new(method, body, connection.build_exclusive_url(path, params), - options, headers, connection.ssl, connection.parallel_manager) + exc_url = connection.build_exclusive_url(path, params, + options.params_encoder) + env = Env.new(method, body, exc_url, options, headers, connection.ssl, + connection.parallel_manager) + env.connection = connection + env end end end diff --git a/spec/faraday/adapter/excon_spec.rb b/spec/faraday/adapter/excon_spec.rb index 796f4ebbf..393c1639d 100644 --- a/spec/faraday/adapter/excon_spec.rb +++ b/spec/faraday/adapter/excon_spec.rb @@ -16,10 +16,42 @@ end context 'config' do - let(:adapter) { Faraday::Adapter::Excon.new } + let(:adapter) { described_class.new } let(:request) { Faraday::RequestOptions.new } let(:uri) { URI.parse('https://example.com') } - let(:env) { { request: request, url: uri } } + let(:env) do + Faraday::Env.from( + request: request, + ssl: Faraday::SSLOptions.new, + url: uri + ) + end + + it 'caches connection' do + # before client is created + env.ssl.client_cert = 'client-cert' + request.boundary = 'doesnt-matter' + + client = adapter.connection(env) + expect(client.data[:ssl_verify_peer]).to eq(true) + expect(client.data[:client_cert]).to eq('client-cert') + expect(client.data[:connect_timeout]).to eq(60) + + # client2 is cached because no important request options are set + client2 = adapter.connection(env) + expect(client2.data[:ssl_verify_peer]).to eq(true) + expect(client2.data[:client_cert]).to eq('client-cert') + expect(client2.data[:connect_timeout]).to eq(60) + expect(client2.object_id).to eq(client.object_id) + + # important request setting, so client3 is new + env.request.timeout = 5 + client3 = adapter.connection(env) + expect(client3.data[:ssl_verify_peer]).to eq(true) + expect(client3.data[:client_cert]).to eq('client-cert') + expect(client3.data[:connect_timeout]).to eq(5) + expect(client3.object_id).not_to eq(client.object_id) + end it 'sets timeout' do request.timeout = 5 diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index c9e805012..5af753b09 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -24,10 +24,38 @@ context 'Options' do let(:request) { Faraday::RequestOptions.new } - let(:env) { { request: request } } - let(:options) { {} } - let(:adapter) { Faraday::Adapter::HTTPClient.new } - let(:client) { adapter.connection(url: URI.parse('https://example.com')) } + let(:env) do + Faraday::Env.from( + request: request, + ssl: Faraday::SSLOptions.new, + url: URI.parse('https://example.com') + ) + end + let(:adapter) { described_class.new } + let(:client) { adapter.connection(env) } + + it 'caches connection' do + # before client is created + env.ssl.client_cert = 'client-cert' + request.boundary = 'doesnt-matter' + + expect(client.ssl_config.client_cert).to eq('client-cert') + expect(client.connect_timeout).to eq(60) + + # client2 is cached because no important request options are set + client2 = adapter.connection(env) + expect(client2.ssl_config.client_cert).to eq('client-cert') + expect(client2.connect_timeout).to eq(60) + expect(client2.object_id).to eq(client.object_id) + + # important request setting, so client3 is new + env.request.timeout = 5 + client3 = adapter.connection(env) + expect(client3.object_id).not_to eq(client2.object_id) + expect(client3.ssl_config.client_cert).to eq('client-cert') + + expect(client3.connect_timeout).to eq(5) + end it 'configures timeout' do assert_default_timeouts! diff --git a/spec/faraday/adapter/net_http_persistent_spec.rb b/spec/faraday/adapter/net_http_persistent_spec.rb index cdf85a134..83ec31481 100644 --- a/spec/faraday/adapter/net_http_persistent_spec.rb +++ b/spec/faraday/adapter/net_http_persistent_spec.rb @@ -5,6 +5,42 @@ it_behaves_like 'an adapter' + context 'config' do + let(:adapter) { described_class.new } + let(:request) { Faraday::RequestOptions.new } + let(:uri) { URI.parse('https://example.com') } + let(:env) do + Faraday::Env.from( + request: request, + ssl: Faraday::SSLOptions.new, + url: uri + ) + end + + it 'caches connection' do + # before client is created + env.ssl.client_cert = 'client-cert' + request.boundary = 'doesnt-matter' + + client = adapter.connection(env) + expect(client.certificate).to eq('client-cert') + expect(client.read_timeout).to be_nil + + # client2 is cached because no important request options are set + client2 = adapter.connection(env) + expect(client2.certificate).to eq('client-cert') + expect(client2.read_timeout).to be_nil + expect(client2.object_id).to eq(client.object_id) + + # important request setting, so client3 is new + env.request.timeout = 5 + client3 = adapter.connection(env) + expect(client3.certificate).to eq('client-cert') + expect(client3.read_timeout).to eq(5) + expect(client3.object_id).not_to eq(client2.object_id) + end + end + it 'allows to provide adapter specific configs' do url = URI('https://example.com') diff --git a/spec/faraday/adapter/patron_spec.rb b/spec/faraday/adapter/patron_spec.rb index 812fd1a06..0e1cdd95d 100644 --- a/spec/faraday/adapter/patron_spec.rb +++ b/spec/faraday/adapter/patron_spec.rb @@ -15,4 +15,49 @@ expect { conn.get('/') }.to raise_error(RuntimeError, 'Configuration block called') end + + context 'config' do + let(:adapter) { described_class.new } + let(:request) { Faraday::RequestOptions.new } + let(:uri) { URI.parse('https://example.com') } + let(:env) do + Faraday::Env.from( + request: request, + ssl: Faraday::SSLOptions.new, + url: uri, + connection: Faraday::ConnectionOptions.new + ) + end + + it 'caches connection' do + # before client is created + env.connection.timeout = 4 + env.connection.open_timeout = 1 + env.ssl.ca_file = 'ca-file' + request.boundary = 'doesnt-matter' + + client = adapter.connection(env) + expect(!!client.insecure).to eq(false) + expect(client.cacert).to eq('ca-file') + expect(client.timeout).to eq(4) + expect(client.connect_timeout).to eq(1) + + # client2 is cached because no important request options are set + client2 = adapter.connection(env) + expect(!!client2.insecure).to eq(false) + expect(client2.cacert).to eq('ca-file') + expect(client2.timeout).to eq(4) + expect(client2.connect_timeout).to eq(1) + expect(client2.object_id).to eq(client.object_id) + + # important request setting, so client3 is new + env.request.timeout = 3 + client3 = adapter.connection(env) + expect(!!client3.insecure).to eq(false) + expect(client3.cacert).to eq('ca-file') + expect(client3.timeout).to eq(3) + expect(client3.connect_timeout).to eq(3) + expect(client3.object_id).not_to eq(client2.object_id) + end + end end diff --git a/spec/faraday/adapter_spec.rb b/spec/faraday/adapter_spec.rb index 22ef1d149..20f8460a7 100644 --- a/spec/faraday/adapter_spec.rb +++ b/spec/faraday/adapter_spec.rb @@ -2,9 +2,51 @@ RSpec.describe Faraday::Adapter do let(:adapter) { Faraday::Adapter.new } - let(:request) { {} } + + context '#reconfigure_connection?' do + let(:all_keys) do + (Faraday::ConnectionOptions.members + Faraday::RequestOptions.members).uniq + end + let(:non_request_keys) { all_keys - Faraday::Adapter::CONNECTION_OPTIONS } + + it 'is not truthy with nil env request value' do + expect(!!reconfigure_connection?(request: nil)).to eq(false) + end + + it 'is not truthy with nil env' do + expect(!!reconfigure_connection?(nil)).to eq(false) + end + + it 'is not truthy with unimportant request keys' do + env = {} + non_request_keys.each do |key| + env[key] = :set + end + env[:request] = env + + expect(!!reconfigure_connection?(env)).to eq(false) + end + + Faraday::Adapter::CONNECTION_OPTIONS.each do |key| + it "is truthy with request #{key.inspect} key" do + env = { request: { key => true } } + expect(!!reconfigure_connection?(env)).to eq(true) + + non_request_keys.each do |k| + env[:request][k] = :set + end + + expect(!!reconfigure_connection?(env)).to eq(true) + end + end + + def reconfigure_connection?(*args) + adapter.send(:reconfigure_connection?, *args) + end + end context '#request_timeout' do + let(:request) { {} } it 'gets :read timeout' do expect(timeout(:read)).to eq(nil)