diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 9c2c85da..97bd46ab 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -77,29 +77,24 @@ def request(verb, uri, options = {}) # rubocop:disable Style/OptionHash end # @overload timeout(options = {}) - # Syntax sugar for `timeout(:per_operation, options)` - # @overload timeout(klass, options = {}) - # Adds a timeout to the request. - # @param [#to_sym] klass - # either :null, :global, or :per_operation + # Adds per operation timeouts to the request # @param [Hash] options # @option options [Float] :read Read timeout # @option options [Float] :write Write timeout # @option options [Float] :connect Connect timeout - def timeout(klass, options = {}) # rubocop:disable Style/OptionHash - if klass.is_a? Hash - options = klass - klass = :per_operation - end - - klass = case klass.to_sym - when :null then HTTP::Timeout::Null - when :global then HTTP::Timeout::Global - when :per_operation then HTTP::Timeout::PerOperation - else raise ArgumentError, "Unsupported Timeout class: #{klass}" - end - - %i[read write connect].each do |k| + # @overload timeout(global_timeout) + # Adds a global timeout to the full request + # @param [Numeric] global_timeout + def timeout(options) + klass, options = case options + when Numeric then [HTTP::Timeout::Global, {:global => options}] + when Hash then [HTTP::Timeout::PerOperation, options] + when :null then [HTTP::Timeout::Null, {}] + else raise ArgumentError, "Use `.timeout(global_timeout_in_seconds)` or `.timeout(connect: x, write: y, read: z)`." + + end + + %i[global read write connect].each do |k| next unless options.key? k options["#{k}_timeout".to_sym] = options.delete k end diff --git a/lib/http/timeout/global.rb b/lib/http/timeout/global.rb index 98b79c50..1566b2a3 100644 --- a/lib/http/timeout/global.rb +++ b/lib/http/timeout/global.rb @@ -3,27 +3,25 @@ require "timeout" require "io/wait" -require "http/timeout/per_operation" +require "http/timeout/null" module HTTP module Timeout - class Global < PerOperation - attr_reader :time_left, :total_timeout - + class Global < Null def initialize(*args) super - reset_counter + + @timeout = @time_left = options.fetch(:global_timeout) end # To future me: Don't remove this again, past you was smarter. def reset_counter - @time_left = connect_timeout + read_timeout + write_timeout - @total_timeout = time_left + @time_left = @timeout end def connect(socket_class, host, port, nodelay = false) reset_timer - ::Timeout.timeout(time_left, TimeoutError) do + ::Timeout.timeout(@time_left, TimeoutError) do @socket = socket_class.open(host, port) @socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) if nodelay end @@ -37,11 +35,11 @@ def connect_ssl begin @socket.connect_nonblock rescue IO::WaitReadable - IO.select([@socket], nil, nil, time_left) + IO.select([@socket], nil, nil, @time_left) log_time retry rescue IO::WaitWritable - IO.select(nil, [@socket], nil, time_left) + IO.select(nil, [@socket], nil, @time_left) log_time retry end @@ -105,13 +103,13 @@ def perform_io # Wait for a socket to become readable def wait_readable_or_timeout - @socket.to_io.wait_readable(time_left) + @socket.to_io.wait_readable(@time_left) log_time end # Wait for a socket to become writable def wait_writable_or_timeout - @socket.to_io.wait_writable(time_left) + @socket.to_io.wait_writable(@time_left) log_time end @@ -123,8 +121,8 @@ def reset_timer def log_time @time_left -= (Time.now - @started) - if time_left <= 0 - raise TimeoutError, "Timed out after using the allocated #{total_timeout} seconds" + if @time_left <= 0 + raise TimeoutError, "Timed out after using the allocated #{@timeout} seconds" end reset_timer diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 413cfd50..0e50ac4c 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -11,8 +11,6 @@ class PerOperation < Null WRITE_TIMEOUT = 0.25 READ_TIMEOUT = 0.25 - attr_reader :read_timeout, :write_timeout, :connect_timeout - def initialize(*args) super @@ -22,7 +20,7 @@ def initialize(*args) end def connect(socket_class, host, port, nodelay = false) - ::Timeout.timeout(connect_timeout, TimeoutError) do + ::Timeout.timeout(@connect_timeout, TimeoutError) do @socket = socket_class.open(host, port) @socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) if nodelay end @@ -67,7 +65,7 @@ def readpartial(size) return :eof if result.nil? return result if result != :wait_readable - raise TimeoutError, "Read timed out after #{read_timeout} seconds" if timeout + raise TimeoutError, "Read timed out after #{@read_timeout} seconds" if timeout # marking the socket for timeout. Why is this not being raised immediately? # it seems there is some race-condition on the network level between calling # #read_nonblock and #wait_readable, in which #read_nonblock signalizes waiting @@ -78,7 +76,7 @@ def readpartial(size) # timeout. Else, the first timeout was a proper timeout. # This hack has to be done because io/wait#wait_readable doesn't provide a value for when # the socket is closed by the server, and HTTP::Parser doesn't provide the limit for the chunks. - timeout = true unless @socket.to_io.wait_readable(read_timeout) + timeout = true unless @socket.to_io.wait_readable(@read_timeout) end end @@ -89,9 +87,9 @@ def write(data) result = @socket.write_nonblock(data, :exception => false) return result unless result == :wait_writable - raise TimeoutError, "Write timed out after #{write_timeout} seconds" if timeout + raise TimeoutError, "Write timed out after #{@write_timeout} seconds" if timeout - timeout = true unless @socket.to_io.wait_writable(write_timeout) + timeout = true unless @socket.to_io.wait_writable(@write_timeout) end end diff --git a/spec/lib/http_spec.rb b/spec/lib/http_spec.rb index cc605181..88d1918b 100644 --- a/spec/lib/http_spec.rb +++ b/spec/lib/http_spec.rb @@ -49,12 +49,12 @@ end context "with a large request body" do - %w[global null per_operation].each do |timeout| - context "with a #{timeout} timeout" do + [:null, 6, {:read => 2, :write => 2, :connect => 2}].each do |timeout| + context "with `.timeout(#{timeout.inspect})`" do [16_000, 16_500, 17_000, 34_000, 68_000].each do |size| [0, rand(0..100), rand(100..1000)].each do |fuzzer| context "with a #{size} body and #{fuzzer} of fuzzing" do - let(:client) { HTTP.timeout(timeout, :read => 2, :write => 2, :connect => 2) } + let(:client) { HTTP.timeout(timeout) } let(:characters) { ("A".."Z").to_a } let(:request_body) do @@ -299,22 +299,8 @@ end describe ".timeout" do - context "without timeout type" do - subject(:client) { HTTP.timeout :read => 123 } - - it "sets timeout_class to PerOperation" do - expect(client.default_options.timeout_class). - to be HTTP::Timeout::PerOperation - end - - it "sets given timeout options" do - expect(client.default_options.timeout_options). - to eq :read_timeout => 123 - end - end - - context "with :null type" do - subject(:client) { HTTP.timeout :null, :read => 123 } + context "specifying a null timeout" do + subject(:client) { HTTP.timeout :null } it "sets timeout_class to Null" do expect(client.default_options.timeout_class). @@ -322,8 +308,8 @@ end end - context "with :per_operation type" do - subject(:client) { HTTP.timeout :per_operation, :read => 123 } + context "specifying per operation timeouts" do + subject(:client) { HTTP.timeout :read => 123 } it "sets timeout_class to PerOperation" do expect(client.default_options.timeout_class). @@ -336,24 +322,19 @@ end end - context "with :global type" do - subject(:client) { HTTP.timeout :global, :read => 123 } + context "specifying a global timeout" do + subject(:client) { HTTP.timeout 123 } it "sets timeout_class to Global" do expect(client.default_options.timeout_class). to be HTTP::Timeout::Global end - it "sets given timeout options" do + it "sets given timeout option" do expect(client.default_options.timeout_options). - to eq :read_timeout => 123 + to eq :global_timeout => 123 end end - - it "fails with unknown timeout type" do - expect { HTTP.timeout(:foobar, :read => 123) }. - to raise_error(ArgumentError, /foobar/) - end end describe ".cookies" do diff --git a/spec/support/http_handling_shared.rb b/spec/support/http_handling_shared.rb index 24344c8c..ce8c7ff9 100644 --- a/spec/support/http_handling_shared.rb +++ b/spec/support/http_handling_shared.rb @@ -1,14 +1,20 @@ # frozen_string_literal: true RSpec.shared_context "HTTP handling" do - describe "timeouts" do - let(:conn_timeout) { 1 } - let(:read_timeout) { 1 } - let(:write_timeout) { 1 } + context "without timeouts" do + let(:options) { {:timeout_class => HTTP::Timeout::Null, :timeout_options => {}} } + + it "works" do + expect(client.get(server.endpoint).body.to_s).to eq("") + end + end + + context "with a per operation timeout" do + let(:response) { client.get(server.endpoint).body.to_s } let(:options) do { - :timeout_class => timeout_class, + :timeout_class => HTTP::Timeout::PerOperation, :timeout_options => { :connect_timeout => conn_timeout, :read_timeout => read_timeout, @@ -16,87 +22,77 @@ } } end + let(:conn_timeout) { 1 } + let(:read_timeout) { 1 } + let(:write_timeout) { 1 } - context "without timeouts" do - let(:timeout_class) { HTTP::Timeout::Null } - let(:conn_timeout) { 0 } - let(:read_timeout) { 0 } - let(:write_timeout) { 0 } - - it "works" do - expect(client.get(server.endpoint).body.to_s).to eq("") - end + it "works" do + expect(response).to eq("") end - context "with a per operation timeout" do - let(:timeout_class) { HTTP::Timeout::PerOperation } - - let(:response) { client.get(server.endpoint).body.to_s } - - it "works" do - expect(response).to eq("") - end + context "connection" do + context "of 1" do + let(:conn_timeout) { 1 } - context "connection" do - context "of 1" do - let(:conn_timeout) { 1 } - - it "does not time out" do - expect { response }.to_not raise_error - end + it "does not time out" do + expect { response }.to_not raise_error end end + end - context "read" do - context "of 0" do - let(:read_timeout) { 0 } + context "read" do + context "of 0" do + let(:read_timeout) { 0 } - it "times out", :flaky do - expect { response }.to raise_error(HTTP::TimeoutError, /Read/i) - end + it "times out", :flaky do + expect { response }.to raise_error(HTTP::TimeoutError, /Read/i) end + end - context "of 2.5" do - let(:read_timeout) { 2.5 } + context "of 2.5" do + let(:read_timeout) { 2.5 } - it "does not time out", :flaky do - expect { client.get("#{server.endpoint}/sleep").body.to_s }.to_not raise_error - end + it "does not time out", :flaky do + expect { client.get("#{server.endpoint}/sleep").body.to_s }.to_not raise_error end end end + end - context "with a global timeout" do - let(:timeout_class) { HTTP::Timeout::Global } - - let(:conn_timeout) { 0 } - let(:read_timeout) { 1 } - let(:write_timeout) { 0 } - - let(:response) { client.get(server.endpoint).body.to_s } + context "with a global timeout" do + let(:options) do + { + :timeout_class => HTTP::Timeout::Global, + :timeout_options => { + :global_timeout => global_timeout + } + } + end + let(:global_timeout) { 1 } - it "errors if connecting takes too long" do - expect(TCPSocket).to receive(:open) do - sleep 1.25 - end + let(:response) { client.get(server.endpoint).body.to_s } - expect { response }.to raise_error(HTTP::TimeoutError, /execution/) + it "errors if connecting takes too long" do + expect(TCPSocket).to receive(:open) do + sleep 1.25 end - it "errors if reading takes too long" do - expect { client.get("#{server.endpoint}/sleep").body.to_s }. - to raise_error(HTTP::TimeoutError, /Timed out/) - end + expect { response }.to raise_error(HTTP::TimeoutError, /execution/) + end - context "it resets state when reusing connections" do - let(:extra_options) { {:persistent => server.endpoint} } + it "errors if reading takes too long" do + expect { client.get("#{server.endpoint}/sleep").body.to_s }. + to raise_error(HTTP::TimeoutError, /Timed out/) + end - let(:read_timeout) { 2.5 } + context "it resets state when reusing connections" do + let(:extra_options) { {:persistent => server.endpoint} } - it "does not timeout", :flaky do - client.get("#{server.endpoint}/sleep").body.to_s - client.get("#{server.endpoint}/sleep").body.to_s - end + let(:global_timeout) { 2.5 } + + it "does not timeout", :flaky do + client.get("#{server.endpoint}/sleep").body.to_s + client.get("#{server.endpoint}/sleep").body.to_s end end end