Simplify setting a timeout

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

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

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
 

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
@@ -249,15 +249,15 @@
 
   describe ".basic_auth" do
     it "fails when options is not a Hash" do
-      expect { HTTP.basic_auth "[FOOBAR]" }.to raise_error
+      expect { HTTP.basic_auth "[FOOBAR]" }.to raise_error(NoMethodError)
     end
 
     it "fails when :pass is not given" do
-      expect { HTTP.basic_auth :user => "[USER]" }.to raise_error
+      expect { HTTP.basic_auth :user => "[USER]" }.to raise_error(KeyError)
     end
 
     it "fails when :user is not given" do
-      expect { HTTP.basic_auth :pass => "[PASS]" }.to raise_error
+      expect { HTTP.basic_auth :pass => "[PASS]" }.to raise_error(KeyError)
     end
 
     it "sets Authorization header with proper BasicAuth value" do
@@ -299,31 +299,17 @@
   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).
           to be HTTP::Timeout::Null
       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