From 25979431e40858c53c069aefe04bc16887ee4393 Mon Sep 17 00:00:00 2001 From: Igor Springer Date: Thu, 18 Jul 2019 11:03:58 +0200 Subject: [PATCH 01/18] Add support for setting Ruby Net::HTTP `read_timeout` option separately Right now `timeout` setting sets all the timeout values (`read`, `open` & `write` if available). To unify the API with `Net::HTTP` one (which has a dedicated method for `read_timeout` as well) I propose extending the `RequestOptions` by `read_timeout`. --- lib/faraday/adapter/net_http.rb | 2 ++ lib/faraday/options/request_options.rb | 5 +++-- spec/faraday/adapter/net_http_spec.rb | 10 ++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index d5c0061d0..4e8799949 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -173,6 +173,8 @@ def configure_request(http, req) end end http.open_timeout = req[:open_timeout] if req[:open_timeout] + http.read_timeout = req[:read_timeout] if req[:read_timeout] + if req[:write_timeout] && http.respond_to?(:write_timeout=) http.write_timeout = req[:write_timeout] end diff --git a/lib/faraday/options/request_options.rb b/lib/faraday/options/request_options.rb index cca000f8b..1a96fb8f9 100644 --- a/lib/faraday/options/request_options.rb +++ b/lib/faraday/options/request_options.rb @@ -3,8 +3,9 @@ module Faraday # RequestOptions contains the configurable properties for a Faraday request. class RequestOptions < Options.new(:params_encoder, :proxy, :bind, - :timeout, :open_timeout, :write_timeout, - :boundary, :oauth, :context, :on_data) + :timeout, :open_timeout, :read_timeout, + :write_timeout, :boundary, :oauth, + :context, :on_data) def []=(key, value) if key && key.to_sym == :proxy diff --git a/spec/faraday/adapter/net_http_spec.rb b/spec/faraday/adapter/net_http_spec.rb index 2197ab180..faf762c8c 100644 --- a/spec/faraday/adapter/net_http_spec.rb +++ b/spec/faraday/adapter/net_http_spec.rb @@ -21,6 +21,16 @@ expect(http.write_timeout).to eq(10) if http.respond_to?(:write_timeout=) end + it 'supports open_timeout' do + adapter.send(:configure_request, http, open_timeout: 10) + + expect(http.open_timeout).to eq(10) + end + it 'supports read_timeout' do + adapter.send(:configure_request, http, read_timeout: 10) + + expect(http.read_timeout).to eq(10) + end context 'with https url' do let(:url) { URI('https://example.com') } From 956c53f99a7c533f7dd29deba0c529c0829cdbde Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 12:01:00 -0600 Subject: [PATCH 02/18] space things out a bit --- spec/faraday/adapter/net_http_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/faraday/adapter/net_http_spec.rb b/spec/faraday/adapter/net_http_spec.rb index faf762c8c..175f806a6 100644 --- a/spec/faraday/adapter/net_http_spec.rb +++ b/spec/faraday/adapter/net_http_spec.rb @@ -11,21 +11,25 @@ let(:http) { adapter.send(:net_http_connection, url: url, request: {}) } it { expect(http.port).to eq(80) } + it 'sets max_retries to 0' do adapter.send(:configure_request, http, {}) expect(http.max_retries).to eq(0) if http.respond_to?(:max_retries=) end + it 'supports write_timeout' do adapter.send(:configure_request, http, write_timeout: 10) expect(http.write_timeout).to eq(10) if http.respond_to?(:write_timeout=) end + it 'supports open_timeout' do adapter.send(:configure_request, http, open_timeout: 10) expect(http.open_timeout).to eq(10) end + it 'supports read_timeout' do adapter.send(:configure_request, http, read_timeout: 10) From dbe4fae178ac1922fa503a6f32113d5bb3cedcba Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 12:20:45 -0600 Subject: [PATCH 03/18] teach Faraday::RequestOptions to get the correct timeout configuration --- lib/faraday/adapter/net_http.rb | 19 ++++----- lib/faraday/options/request_options.rb | 20 +++++++++ spec/faraday/options/request_options_spec.rb | 44 +++++++++++++++++++- 3 files changed, 71 insertions(+), 12 deletions(-) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index 4e8799949..c1e0aaed8 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -165,19 +165,16 @@ def configure_ssl(http, ssl) end def configure_request(http, req) - if req[:timeout] - http.read_timeout = req[:timeout] - http.open_timeout = req[:timeout] - if http.respond_to?(:write_timeout=) - http.write_timeout = req[:timeout] - end + if sec = req.fetch_timeout(:read) + http.read_timeout = sec end - http.open_timeout = req[:open_timeout] if req[:open_timeout] - http.read_timeout = req[:read_timeout] if req[:read_timeout] - - if req[:write_timeout] && http.respond_to?(:write_timeout=) - http.write_timeout = req[:write_timeout] + if sec = http.respond_to?(:write_timeout=) && req.fetch_timeout(:write) + http.write_timeout = sec end + if sec = req.fetch_timeout(:open) + 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=) diff --git a/lib/faraday/options/request_options.rb b/lib/faraday/options/request_options.rb index 1a96fb8f9..08b73dd93 100644 --- a/lib/faraday/options/request_options.rb +++ b/lib/faraday/options/request_options.rb @@ -15,8 +15,28 @@ def []=(key, value) end end + # Fetches either a read, write, or open timeout setting. Defaults to the + # :timeout value if a more specific one is not given. + # + # @param type [Symbol] Describes which timeout setting to get: :read, + # :write, or :open. + # + # @return [Integer, nil] Timeout duration in seconds, or nil if no timeout + # has been set. + def fetch_timeout(type) + unless TIMEOUT_TYPES.include?(type) + raise ArgumentError, "Expected :read, :write, :open. Got #{type.inspect} :(" + end + + self["#{type}_timeout".to_sym] || self[:timeout] + end + def stream_response? on_data.is_a?(Proc) end + + private + + TIMEOUT_TYPES = Set.new([:read, :write, :open]) end end diff --git a/spec/faraday/options/request_options_spec.rb b/spec/faraday/options/request_options_spec.rb index d73c7a89a..80791e8d3 100644 --- a/spec/faraday/options/request_options_spec.rb +++ b/spec/faraday/options/request_options_spec.rb @@ -1,8 +1,50 @@ # frozen_string_literal: true RSpec.describe Faraday::RequestOptions do + subject(:options) { Faraday::RequestOptions.new } + + context '#fetch_timeout' do + it 'gets :read timeout' do + expect(options.fetch_timeout(:read)).to eq(nil) + + options[:timeout] = 5 + options[:write_timeout] = 1 + + expect(options.fetch_timeout(:read)).to eq(5) + + options[:read_timeout] = 2 + + expect(options.fetch_timeout(:read)).to eq(2) + end + + it 'gets :open timeout' do + expect(options.fetch_timeout(:open)).to eq(nil) + + options[:timeout] = 5 + options[:write_timeout] = 1 + + expect(options.fetch_timeout(:open)).to eq(5) + + options[:open_timeout] = 2 + + expect(options.fetch_timeout(:open)).to eq(2) + end + + it 'gets :write timeout' do + expect(options.fetch_timeout(:write)).to eq(nil) + + options[:timeout] = 5 + options[:read_timeout] = 1 + + expect(options.fetch_timeout(:write)).to eq(5) + + options[:write_timeout] = 2 + + expect(options.fetch_timeout(:write)).to eq(2) + end + end + it 'allows to set the request proxy' do - options = Faraday::RequestOptions.new expect(options.proxy).to be_nil expect { options[:proxy] = { booya: 1 } }.to raise_error(NoMethodError) From abade0d4530b858a76ae5ac73ed8d53227fb81d1 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 15:32:22 -0600 Subject: [PATCH 04/18] test current EMHttp timeout config --- spec/faraday/adapter/em_http_spec.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/faraday/adapter/em_http_spec.rb b/spec/faraday/adapter/em_http_spec.rb index efe61348d..7f6a86dd9 100644 --- a/spec/faraday/adapter/em_http_spec.rb +++ b/spec/faraday/adapter/em_http_spec.rb @@ -13,4 +13,32 @@ expect(req.connopts.inactivity_timeout).to eq(20) end + + context 'Options' do + let(:request) { Faraday::RequestOptions.new } + let(:env) { { request: request } } + let(:adapter) { + Object.new.tap do |o| + class << o + include Faraday::Adapter::EMHttp::Options + end + end + } + let(:options) { {} } + + it 'configures timeout' do + request.timeout = 5 + adapter.configure_timeout(options, env) + expect(options[:inactivity_timeout]).to eq(5) + expect(options[:connect_timeout]).to eq(5) + end + + it 'configures timeout and open_timeout' do + request.timeout = 5 + request.open_timeout = 1 + adapter.configure_timeout(options, env) + expect(options[:inactivity_timeout]).to eq(5) + expect(options[:connect_timeout]).to eq(1) + end + end end From 24cf49c216d8efbe4dfffe42f0c7ae738d4d8c31 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 15:40:36 -0600 Subject: [PATCH 05/18] teach EMHttp adapter to use #fetch_timeout --- lib/faraday/adapter/em_http.rb | 7 +++---- spec/faraday/adapter/em_http_spec.rb | 13 ++++++++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/faraday/adapter/em_http.rb b/lib/faraday/adapter/em_http.rb index 897a8ef75..f405ffab7 100644 --- a/lib/faraday/adapter/em_http.rb +++ b/lib/faraday/adapter/em_http.rb @@ -70,10 +70,9 @@ def configure_ssl(options, env) # Reads out timeout settings from env into options def configure_timeout(options, env) - timeout, open_timeout = request_options(env) - .values_at(:timeout, :open_timeout) - options[:connect_timeout] = options[:inactivity_timeout] = timeout - options[:connect_timeout] = open_timeout if open_timeout + req = request_options(env) + options[:inactivity_timeout] = req.fetch_timeout(:read) + options[:connect_timeout] = req.fetch_timeout(:open) end # Reads out compression header settings from env into options diff --git a/spec/faraday/adapter/em_http_spec.rb b/spec/faraday/adapter/em_http_spec.rb index 7f6a86dd9..bb3aaecf9 100644 --- a/spec/faraday/adapter/em_http_spec.rb +++ b/spec/faraday/adapter/em_http_spec.rb @@ -6,10 +6,12 @@ it_behaves_like 'an adapter' + let(:request) { Faraday::RequestOptions.new } + it 'allows to provide adapter specific configs' do url = URI('https://example.com:1234') adapter = described_class.new nil, inactivity_timeout: 20 - req = adapter.create_request(url: url, request: {}) + req = adapter.create_request(url: url, request: request) expect(req.connopts.inactivity_timeout).to eq(20) end @@ -40,5 +42,14 @@ class << o expect(options[:inactivity_timeout]).to eq(5) expect(options[:connect_timeout]).to eq(1) end + + it 'configures all timeout settings' do + request.timeout = 5 + request.read_timeout = 3 + request.open_timeout = 1 + adapter.configure_timeout(options, env) + expect(options[:inactivity_timeout]).to eq(3) + expect(options[:connect_timeout]).to eq(1) + end end end From 6c117f47b981d64d991e9cad8dfb5655c6038fc7 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 15:50:25 -0600 Subject: [PATCH 06/18] test current Excon timeout config --- spec/faraday/adapter/excon_spec.rb | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spec/faraday/adapter/excon_spec.rb b/spec/faraday/adapter/excon_spec.rb index fee13d052..e08df06b3 100644 --- a/spec/faraday/adapter/excon_spec.rb +++ b/spec/faraday/adapter/excon_spec.rb @@ -14,4 +14,36 @@ expect(conn.data[:debug_request]).to be_truthy end + + context "config" do + let(:adapter) { Faraday::Adapter::Excon.new } + let(:request) { Faraday::RequestOptions.new } + let(:uri) { URI.parse('https://example.com') } + let(:env) { { request: request, url: uri } } + + it "sets timeout" do + request.timeout = 5 + options = adapter.send(:opts_from_env, env) + expect(options[:read_timeout]).to eq(5) + expect(options[:write_timeout]).to eq(5) + expect(options[:connect_timeout]).to eq(5) + end + + it "sets timeout and open_timeout" do + request.timeout = 5 + request.open_timeout = 3 + options = adapter.send(:opts_from_env, env) + expect(options[:read_timeout]).to eq(5) + expect(options[:write_timeout]).to eq(5) + expect(options[:connect_timeout]).to eq(3) + end + + it "sets open_timeout" do + request.open_timeout = 3 + options = adapter.send(:opts_from_env, env) + expect(options[:read_timeout]).to eq(nil) + expect(options[:write_timeout]).to eq(nil) + expect(options[:connect_timeout]).to eq(nil) + end + end end From a5a398c2319bece0b00bd1a5e02f54929b8677e2 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 15:50:50 -0600 Subject: [PATCH 07/18] teach Excon to use #fetch_timeout --- lib/faraday/adapter/excon.rb | 20 +++++++++----------- spec/faraday/adapter/excon_spec.rb | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index b0ea0d7d7..03be734d9 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -90,17 +90,15 @@ def amend_opts_with_ssl!(opts, ssl) end def amend_opts_with_timeouts!(opts, req) - timeout = req[:timeout] - return unless timeout - - opts[:read_timeout] = timeout - opts[:connect_timeout] = timeout - opts[:write_timeout] = timeout - - open_timeout = req[:open_timeout] - return unless open_timeout - - opts[:connect_timeout] = open_timeout + if sec = req.fetch_timeout(:read) + opts[:read_timeout] = sec + end + if sec = req.fetch_timeout(:write) + opts[:write_timeout] = sec + end + if sec = req.fetch_timeout(:open) + opts[:connect_timeout] = sec + end end def amend_opts_with_proxy_settings!(opts, req) diff --git a/spec/faraday/adapter/excon_spec.rb b/spec/faraday/adapter/excon_spec.rb index e08df06b3..b5064a6b0 100644 --- a/spec/faraday/adapter/excon_spec.rb +++ b/spec/faraday/adapter/excon_spec.rb @@ -43,7 +43,7 @@ options = adapter.send(:opts_from_env, env) expect(options[:read_timeout]).to eq(nil) expect(options[:write_timeout]).to eq(nil) - expect(options[:connect_timeout]).to eq(nil) + expect(options[:connect_timeout]).to eq(3) end end end From 8dae8759062288a82084194e7be7fe563b617979 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 16:05:06 -0600 Subject: [PATCH 08/18] lint --- lib/faraday/adapter/excon.rb | 12 +++++++----- lib/faraday/adapter/net_http.rb | 7 ++++--- lib/faraday/options/request_options.rb | 7 +++---- spec/faraday/adapter/em_http_spec.rb | 8 +------- spec/faraday/adapter/excon_spec.rb | 8 ++++---- 5 files changed, 19 insertions(+), 23 deletions(-) diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index 03be734d9..b9b8d090b 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -90,15 +90,17 @@ def amend_opts_with_ssl!(opts, ssl) end def amend_opts_with_timeouts!(opts, req) - if sec = req.fetch_timeout(:read) + if (sec = req.fetch_timeout(:read)) opts[:read_timeout] = sec end - if sec = req.fetch_timeout(:write) + + if (sec = req.fetch_timeout(:write)) opts[:write_timeout] = sec end - if sec = req.fetch_timeout(:open) - opts[:connect_timeout] = sec - end + + return unless (sec = req.fetch_timeout(:open)) + + opts[:connect_timeout] = sec end def amend_opts_with_proxy_settings!(opts, req) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index c1e0aaed8..451807536 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -165,13 +165,14 @@ def configure_ssl(http, ssl) end def configure_request(http, req) - if sec = req.fetch_timeout(:read) + if (sec = req.fetch_timeout(:read)) http.read_timeout = sec end - if sec = http.respond_to?(:write_timeout=) && req.fetch_timeout(:write) + if (sec = http.respond_to?(:write_timeout=) && + req.fetch_timeout(:write)) http.write_timeout = sec end - if sec = req.fetch_timeout(:open) + if (sec = req.fetch_timeout(:open)) http.open_timeout = sec end diff --git a/lib/faraday/options/request_options.rb b/lib/faraday/options/request_options.rb index 08b73dd93..b8fbf41e5 100644 --- a/lib/faraday/options/request_options.rb +++ b/lib/faraday/options/request_options.rb @@ -25,7 +25,8 @@ def []=(key, value) # has been set. def fetch_timeout(type) unless TIMEOUT_TYPES.include?(type) - raise ArgumentError, "Expected :read, :write, :open. Got #{type.inspect} :(" + msg = "Expected :read, :write, :open. Got #{type.inspect} :(" + raise ArgumentError, msg end self["#{type}_timeout".to_sym] || self[:timeout] @@ -35,8 +36,6 @@ def stream_response? on_data.is_a?(Proc) end - private - - TIMEOUT_TYPES = Set.new([:read, :write, :open]) + TIMEOUT_TYPES = Set.new(%i[read write open]) end end diff --git a/spec/faraday/adapter/em_http_spec.rb b/spec/faraday/adapter/em_http_spec.rb index bb3aaecf9..9be3d67e7 100644 --- a/spec/faraday/adapter/em_http_spec.rb +++ b/spec/faraday/adapter/em_http_spec.rb @@ -19,14 +19,8 @@ context 'Options' do let(:request) { Faraday::RequestOptions.new } let(:env) { { request: request } } - let(:adapter) { - Object.new.tap do |o| - class << o - include Faraday::Adapter::EMHttp::Options - end - end - } let(:options) { {} } + let(:adapter) { Faraday::Adapter::EMHttp.new } it 'configures timeout' do request.timeout = 5 diff --git a/spec/faraday/adapter/excon_spec.rb b/spec/faraday/adapter/excon_spec.rb index b5064a6b0..d71ef8c44 100644 --- a/spec/faraday/adapter/excon_spec.rb +++ b/spec/faraday/adapter/excon_spec.rb @@ -15,13 +15,13 @@ expect(conn.data[:debug_request]).to be_truthy end - context "config" do + context 'config' do let(:adapter) { Faraday::Adapter::Excon.new } let(:request) { Faraday::RequestOptions.new } let(:uri) { URI.parse('https://example.com') } let(:env) { { request: request, url: uri } } - it "sets timeout" do + it 'sets timeout' do request.timeout = 5 options = adapter.send(:opts_from_env, env) expect(options[:read_timeout]).to eq(5) @@ -29,7 +29,7 @@ expect(options[:connect_timeout]).to eq(5) end - it "sets timeout and open_timeout" do + it 'sets timeout and open_timeout' do request.timeout = 5 request.open_timeout = 3 options = adapter.send(:opts_from_env, env) @@ -38,7 +38,7 @@ expect(options[:connect_timeout]).to eq(3) end - it "sets open_timeout" do + it 'sets open_timeout' do request.open_timeout = 3 options = adapter.send(:opts_from_env, env) expect(options[:read_timeout]).to eq(nil) From b35f6a24809830d5c039725bcb5f6bff6355c646 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 16:39:39 -0600 Subject: [PATCH 09/18] RequestOptions#fetch_timeout => Adapter#request_timeout This allows adapter internals and specs to treat request options as a plain hash. --- lib/faraday/adapter.rb | 22 +++++++++ lib/faraday/adapter/em_http.rb | 4 +- lib/faraday/adapter/excon.rb | 6 +-- lib/faraday/adapter/net_http.rb | 6 +-- lib/faraday/options/request_options.rb | 17 ------- spec/faraday/adapter/em_http_spec.rb | 4 +- spec/faraday/adapter_spec.rb | 51 ++++++++++++++++++++ spec/faraday/options/request_options_spec.rb | 41 ---------------- 8 files changed, 82 insertions(+), 69 deletions(-) create mode 100644 spec/faraday/adapter_spec.rb diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index 6a3bb7d5c..c0fb14185 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -65,5 +65,27 @@ def save_response(env, status, body, headers = nil, reason_phrase = nil) env.response.finish(env) unless env.parallel? env.response end + + # Fetches either a read, write, or open timeout setting. Defaults to the + # :timeout value if a more specific one is not given. + # + # @param type [Symbol] Describes which timeout setting to get: :read, + # :write, or :open. + # @param options [Hash] Hash containing Symbol keys like :timeout, + # :read_timeout, :write_timeout, :open_timeout, or + # :timeout + # + # @return [Integer, nil] Timeout duration in seconds, or nil if no timeout + # has been set. + def request_timeout(type, options) + unless TIMEOUT_TYPES.include?(type) + msg = "Expected :read, :write, :open. Got #{type.inspect} :(" + raise ArgumentError, msg + end + + options["#{type}_timeout".to_sym] || options[:timeout] + end + + TIMEOUT_TYPES = Set.new(%i[read write open]) end end diff --git a/lib/faraday/adapter/em_http.rb b/lib/faraday/adapter/em_http.rb index f405ffab7..acadc35ff 100644 --- a/lib/faraday/adapter/em_http.rb +++ b/lib/faraday/adapter/em_http.rb @@ -71,8 +71,8 @@ def configure_ssl(options, env) # Reads out timeout settings from env into options def configure_timeout(options, env) req = request_options(env) - options[:inactivity_timeout] = req.fetch_timeout(:read) - options[:connect_timeout] = req.fetch_timeout(:open) + options[:inactivity_timeout] = request_timeout(:read, req) + options[:connect_timeout] = request_timeout(:open, req) end # Reads out compression header settings from env into options diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index b9b8d090b..ad8bb8ca8 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -90,15 +90,15 @@ def amend_opts_with_ssl!(opts, ssl) end def amend_opts_with_timeouts!(opts, req) - if (sec = req.fetch_timeout(:read)) + if (sec = request_timeout(:read, req)) opts[:read_timeout] = sec end - if (sec = req.fetch_timeout(:write)) + if (sec = request_timeout(:write, req)) opts[:write_timeout] = sec end - return unless (sec = req.fetch_timeout(:open)) + return unless (sec = request_timeout(:open, req)) opts[:connect_timeout] = sec end diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index 451807536..afdfa87b5 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -165,14 +165,14 @@ def configure_ssl(http, ssl) end def configure_request(http, req) - if (sec = req.fetch_timeout(:read)) + if (sec = request_timeout(:read, req)) http.read_timeout = sec end if (sec = http.respond_to?(:write_timeout=) && - req.fetch_timeout(:write)) + request_timeout(:write, req)) http.write_timeout = sec end - if (sec = req.fetch_timeout(:open)) + if (sec = request_timeout(:open, req)) http.open_timeout = sec end diff --git a/lib/faraday/options/request_options.rb b/lib/faraday/options/request_options.rb index b8fbf41e5..dbe1d7ead 100644 --- a/lib/faraday/options/request_options.rb +++ b/lib/faraday/options/request_options.rb @@ -15,23 +15,6 @@ def []=(key, value) end end - # Fetches either a read, write, or open timeout setting. Defaults to the - # :timeout value if a more specific one is not given. - # - # @param type [Symbol] Describes which timeout setting to get: :read, - # :write, or :open. - # - # @return [Integer, nil] Timeout duration in seconds, or nil if no timeout - # has been set. - def fetch_timeout(type) - unless TIMEOUT_TYPES.include?(type) - msg = "Expected :read, :write, :open. Got #{type.inspect} :(" - raise ArgumentError, msg - end - - self["#{type}_timeout".to_sym] || self[:timeout] - end - def stream_response? on_data.is_a?(Proc) end diff --git a/spec/faraday/adapter/em_http_spec.rb b/spec/faraday/adapter/em_http_spec.rb index 9be3d67e7..74a287853 100644 --- a/spec/faraday/adapter/em_http_spec.rb +++ b/spec/faraday/adapter/em_http_spec.rb @@ -6,12 +6,10 @@ it_behaves_like 'an adapter' - let(:request) { Faraday::RequestOptions.new } - it 'allows to provide adapter specific configs' do url = URI('https://example.com:1234') adapter = described_class.new nil, inactivity_timeout: 20 - req = adapter.create_request(url: url, request: request) + req = adapter.create_request(url: url, request: {}) expect(req.connopts.inactivity_timeout).to eq(20) end diff --git a/spec/faraday/adapter_spec.rb b/spec/faraday/adapter_spec.rb new file mode 100644 index 000000000..61e056df8 --- /dev/null +++ b/spec/faraday/adapter_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +RSpec.describe Faraday::Adapter do + let(:adapter) { Faraday::Adapter.new } + let(:request) { {} } + + context '#request_timeout' do + it 'gets :read timeout' do + expect(timeout(:read)).to eq(nil) + + request[:timeout] = 5 + request[:write_timeout] = 1 + + expect(timeout(:read)).to eq(5) + + request[:read_timeout] = 2 + + expect(timeout(:read)).to eq(2) + end + + it 'gets :open timeout' do + expect(timeout(:open)).to eq(nil) + + request[:timeout] = 5 + request[:write_timeout] = 1 + + expect(timeout(:open)).to eq(5) + + request[:open_timeout] = 2 + + expect(timeout(:open)).to eq(2) + end + + it 'gets :write timeout' do + expect(timeout(:write)).to eq(nil) + + request[:timeout] = 5 + request[:read_timeout] = 1 + + expect(timeout(:write)).to eq(5) + + request[:write_timeout] = 2 + + expect(timeout(:write)).to eq(2) + end + + def timeout(type) + adapter.send(:request_timeout, type, request) + end + end +end diff --git a/spec/faraday/options/request_options_spec.rb b/spec/faraday/options/request_options_spec.rb index 80791e8d3..8c1bb9921 100644 --- a/spec/faraday/options/request_options_spec.rb +++ b/spec/faraday/options/request_options_spec.rb @@ -3,47 +3,6 @@ RSpec.describe Faraday::RequestOptions do subject(:options) { Faraday::RequestOptions.new } - context '#fetch_timeout' do - it 'gets :read timeout' do - expect(options.fetch_timeout(:read)).to eq(nil) - - options[:timeout] = 5 - options[:write_timeout] = 1 - - expect(options.fetch_timeout(:read)).to eq(5) - - options[:read_timeout] = 2 - - expect(options.fetch_timeout(:read)).to eq(2) - end - - it 'gets :open timeout' do - expect(options.fetch_timeout(:open)).to eq(nil) - - options[:timeout] = 5 - options[:write_timeout] = 1 - - expect(options.fetch_timeout(:open)).to eq(5) - - options[:open_timeout] = 2 - - expect(options.fetch_timeout(:open)).to eq(2) - end - - it 'gets :write timeout' do - expect(options.fetch_timeout(:write)).to eq(nil) - - options[:timeout] = 5 - options[:read_timeout] = 1 - - expect(options.fetch_timeout(:write)).to eq(5) - - options[:write_timeout] = 2 - - expect(options.fetch_timeout(:write)).to eq(2) - end - end - it 'allows to set the request proxy' do expect(options.proxy).to be_nil From 55c0b742ec899d612f32874ede331e82f067fd2f Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 11:19:48 -0600 Subject: [PATCH 10/18] test current httpclient timeout behavior --- spec/faraday/adapter/httpclient_spec.rb | 53 +++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index d3fb5f0e5..29a7c038f 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -1,6 +1,11 @@ # frozen_string_literal: true RSpec.describe Faraday::Adapter::HTTPClient do + # ruby gem defaults for testing purposes + HTTPCLIENT_OPEN = 60 + HTTPCLIENT_READ = 60 + HTTPCLIENT_WRITE = 120 + features :request_body_on_query_methods, :reason_phrase_parse, :compression, :trace_method, :connect_method, :local_socket_binding @@ -18,4 +23,52 @@ expect(client.keep_alive_timeout).to eq(20) expect(client.ssl_config.timeout).to eq(25) end + + context 'Options' do + let(:request) { Faraday::RequestOptions.new } + let(:env) { { request: request } } + let(:options) { {} } + let(:adapter) { Faraday::Adapter::HTTPClient.new } + let(:client) { adapter.client } + + it 'configures timeout' do + assert_default_timeouts! + + request.timeout = 5 + adapter.configure_timeouts(request) + + expect(client.connect_timeout).to eq(5) + expect(client.send_timeout).to eq(5) + expect(client.receive_timeout).to eq(5) + end + + it 'configures open timeout' do + assert_default_timeouts! + + request.open_timeout = 1 + adapter.configure_timeouts(request) + + expect(client.connect_timeout).to eq(1) + expect(client.send_timeout).to eq(1) + expect(client.receive_timeout).to eq(HTTPCLIENT_READ) + end + + it 'configures multiple timeouts' do + assert_default_timeouts! + + request.open_timeout = 1 + request.timeout = 5 + adapter.configure_timeouts(request) + + expect(client.connect_timeout).to eq(1) + expect(client.send_timeout).to eq(1) + expect(client.receive_timeout).to eq(5) + end + + def assert_default_timeouts! + expect(client.connect_timeout).to eq(HTTPCLIENT_OPEN) + expect(client.send_timeout).to eq(HTTPCLIENT_WRITE) + expect(client.receive_timeout).to eq(HTTPCLIENT_READ) + end + end end From 711c257295758c919c03e6a9a693bc73644d260f Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:03:29 -0600 Subject: [PATCH 11/18] teach httpclient adapter how to use #request_timeout --- lib/faraday/adapter.rb | 12 ++++++++---- lib/faraday/adapter/httpclient.rb | 20 +++++++++----------- spec/faraday/adapter/httpclient_spec.rb | 7 ++++--- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index c0fb14185..a8e9c1839 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -78,14 +78,18 @@ def save_response(env, status, body, headers = nil, reason_phrase = nil) # @return [Integer, nil] Timeout duration in seconds, or nil if no timeout # has been set. def request_timeout(type, options) - unless TIMEOUT_TYPES.include?(type) + key = TIMEOUT_KEYS[type] + if key.nil? msg = "Expected :read, :write, :open. Got #{type.inspect} :(" raise ArgumentError, msg end - - options["#{type}_timeout".to_sym] || options[:timeout] + options[key] || options[:timeout] end - TIMEOUT_TYPES = Set.new(%i[read write open]) + TIMEOUT_KEYS = { + read: :read_timeout, + open: :open_timeout, + write: :write_timeout, + } end end diff --git a/lib/faraday/adapter/httpclient.rb b/lib/faraday/adapter/httpclient.rb index e0ac23c7e..bd5a2d480 100644 --- a/lib/faraday/adapter/httpclient.rb +++ b/lib/faraday/adapter/httpclient.rb @@ -101,19 +101,17 @@ def configure_ssl(ssl) # @param req [Hash] def configure_timeouts(req) - configure_timeout(req) if req[:timeout] - configure_open_timeout(req) if req[:open_timeout] - end + if (sec = request_timeout(:open, req)) + client.connect_timeout = sec + end - def configure_timeout(req) - client.connect_timeout = req[:timeout] - client.receive_timeout = req[:timeout] - client.send_timeout = req[:timeout] - end + if (sec = request_timeout(:write, req)) + client.send_timeout = sec + end - def configure_open_timeout(req) - client.connect_timeout = req[:open_timeout] - client.send_timeout = req[:open_timeout] + if (sec = request_timeout(:read, req)) + client.receive_timeout = sec + end end def configure_client diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index 29a7c038f..cbc715562 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -49,7 +49,7 @@ adapter.configure_timeouts(request) expect(client.connect_timeout).to eq(1) - expect(client.send_timeout).to eq(1) + expect(client.send_timeout).to eq(HTTPCLIENT_WRITE) expect(client.receive_timeout).to eq(HTTPCLIENT_READ) end @@ -57,11 +57,12 @@ assert_default_timeouts! request.open_timeout = 1 - request.timeout = 5 + request.write_timeout = 10 + request.read_timeout = 5 adapter.configure_timeouts(request) expect(client.connect_timeout).to eq(1) - expect(client.send_timeout).to eq(1) + expect(client.send_timeout).to eq(10) expect(client.receive_timeout).to eq(5) end From 6394bc08a5012c83a583030bd7f47c5883301379 Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:06:07 -0600 Subject: [PATCH 12/18] teach patron adapter how to use #request_timeout --- lib/faraday/adapter/patron.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/faraday/adapter/patron.rb b/lib/faraday/adapter/patron.rb index 3020dcf4b..3d1ed6f83 100644 --- a/lib/faraday/adapter/patron.rb +++ b/lib/faraday/adapter/patron.rb @@ -18,10 +18,13 @@ def call(env) end if (req = env[:request]) - if req[:timeout] - session.timeout = session.connect_timeout = req[:timeout] + if (sec = request_timeout(:read, req)) + session.timeout = sec + end + + if (sec = request_timeout(:open, req)) + session.connect_timeout = sec end - session.connect_timeout = req[:open_timeout] if req[:open_timeout] if (proxy = req[:proxy]) proxy_uri = proxy[:uri].dup From 0aea513d72c5d5b061c2e7bd60815cf2751f9ec2 Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:07:42 -0600 Subject: [PATCH 13/18] teach rack adapter to use #request_timeout --- lib/faraday/adapter/rack.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/faraday/adapter/rack.rb b/lib/faraday/adapter/rack.rb index c866792b9..6a237ee13 100644 --- a/lib/faraday/adapter/rack.rb +++ b/lib/faraday/adapter/rack.rb @@ -37,7 +37,8 @@ def call(env) rack_env[name] = value end - timeout = env[:request][:timeout] || env[:request][:open_timeout] + timeout = request_timeout(:open, env[:request]) + timeout ||= request_timeout(:read, env[:request]) response = if timeout Timer.timeout(timeout, Faraday::TimeoutError) do execute_request(env, rack_env) From 18c204e3a5f88b088f5353e9efab66cfa7f6890f Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:18:03 -0600 Subject: [PATCH 14/18] lint fixes --- lib/faraday/adapter.rb | 4 +-- lib/faraday/adapter/httpclient.rb | 6 ++--- lib/faraday/adapter/patron.rb | 41 +++++++++++++++++++------------ 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index a8e9c1839..83103001e 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -89,7 +89,7 @@ def request_timeout(type, options) TIMEOUT_KEYS = { read: :read_timeout, open: :open_timeout, - write: :write_timeout, - } + write: :write_timeout + }.freeze end end diff --git a/lib/faraday/adapter/httpclient.rb b/lib/faraday/adapter/httpclient.rb index bd5a2d480..45e0d585a 100644 --- a/lib/faraday/adapter/httpclient.rb +++ b/lib/faraday/adapter/httpclient.rb @@ -109,9 +109,9 @@ def configure_timeouts(req) client.send_timeout = sec end - if (sec = request_timeout(:read, req)) - client.receive_timeout = sec - end + return unless (sec = request_timeout(:read, req)) + + client.receive_timeout = sec end def configure_client diff --git a/lib/faraday/adapter/patron.rb b/lib/faraday/adapter/patron.rb index 3d1ed6f83..a009c50f6 100644 --- a/lib/faraday/adapter/patron.rb +++ b/lib/faraday/adapter/patron.rb @@ -18,22 +18,8 @@ def call(env) end if (req = env[:request]) - if (sec = request_timeout(:read, req)) - session.timeout = sec - end - - if (sec = request_timeout(:open, req)) - session.connect_timeout = sec - end - - if (proxy = req[:proxy]) - proxy_uri = proxy[:uri].dup - proxy_uri.user = proxy[:user] && - Utils.escape(proxy[:user]).gsub('+', '%20') - proxy_uri.password = proxy[:password] && - Utils.escape(proxy[:password]).gsub('+', '%20') - session.proxy = proxy_uri.to_s - end + configure_timeouts(session, req) + configure_proxy(session, req[:proxy]) end response = begin @@ -94,6 +80,29 @@ def configure_ssl(session, ssl) end end + def configure_timeouts(session, req) + return unless req + + if (sec = request_timeout(:read, req)) + session.timeout = sec + end + + return unless (sec = request_timeout(:open, req)) + + session.connect_timeout = sec + end + + def configure_proxy(session, proxy) + return unless proxy + + proxy_uri = proxy[:uri].dup + proxy_uri.user = proxy[:user] && + Utils.escape(proxy[:user]).gsub('+', '%20') + proxy_uri.password = proxy[:password] && + Utils.escape(proxy[:password]).gsub('+', '%20') + session.proxy = proxy_uri.to_s + end + private CURL_TIMEOUT_MESSAGES = [ From 75b9804e334a6a3ebec702e44a09f72d0f1abb0c Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:21:19 -0600 Subject: [PATCH 15/18] unused --- lib/faraday/options/request_options.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/faraday/options/request_options.rb b/lib/faraday/options/request_options.rb index dbe1d7ead..1a96fb8f9 100644 --- a/lib/faraday/options/request_options.rb +++ b/lib/faraday/options/request_options.rb @@ -18,7 +18,5 @@ def []=(key, value) def stream_response? on_data.is_a?(Proc) end - - TIMEOUT_TYPES = Set.new(%i[read write open]) end end From 296f336b62cd71fe7eb61d5cba189f115e1a790d Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 15 Oct 2019 10:35:03 -0600 Subject: [PATCH 16/18] Update lib/faraday/adapter/net_http.rb Co-Authored-By: Olle Jonsson --- lib/faraday/adapter/net_http.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index afdfa87b5..3ac909602 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -168,6 +168,7 @@ def configure_request(http, req) if (sec = request_timeout(:read, req)) http.read_timeout = sec end + if (sec = http.respond_to?(:write_timeout=) && request_timeout(:write, req)) http.write_timeout = sec From a91581a04eedd2b46cd2cf06aabd34f245f1b238 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 15 Oct 2019 10:35:10 -0600 Subject: [PATCH 17/18] Update lib/faraday/adapter/net_http.rb Co-Authored-By: Olle Jonsson --- lib/faraday/adapter/net_http.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index 3ac909602..1064c0a0e 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -173,6 +173,7 @@ def configure_request(http, req) request_timeout(:write, req)) http.write_timeout = sec end + if (sec = request_timeout(:open, req)) http.open_timeout = sec end From 68c53c448f3d8c8375961ea82347ed7d06afe563 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 17 Oct 2019 10:05:09 -0600 Subject: [PATCH 18/18] prefer Hash#fetch --- lib/faraday/adapter.rb | 3 +-- spec/faraday/adapter_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index 83103001e..959a98817 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -78,8 +78,7 @@ def save_response(env, status, body, headers = nil, reason_phrase = nil) # @return [Integer, nil] Timeout duration in seconds, or nil if no timeout # has been set. def request_timeout(type, options) - key = TIMEOUT_KEYS[type] - if key.nil? + key = TIMEOUT_KEYS.fetch(type) do msg = "Expected :read, :write, :open. Got #{type.inspect} :(" raise ArgumentError, msg end diff --git a/spec/faraday/adapter_spec.rb b/spec/faraday/adapter_spec.rb index 61e056df8..22ef1d149 100644 --- a/spec/faraday/adapter_spec.rb +++ b/spec/faraday/adapter_spec.rb @@ -44,6 +44,10 @@ expect(timeout(:write)).to eq(2) end + it 'attempts unknown timeout type' do + expect { timeout(:unknown) }.to raise_error(ArgumentError) + end + def timeout(type) adapter.send(:request_timeout, type, request) end