Allow combining global and per-operation timeouts

sferik · sferik · commit 67961da1e87f · 2026-03-08T09:13:48.000-07:00
Global and per-operation timeouts are no longer mutually exclusive. `HTTP.timeout(global: 60, read: 30, write: 30, connect: 5)` now sets a global request timeout while also enforcing individual operation limits. The effective timeout for each wait is the minimum of the per-operation value and the remaining global time. Closes #773.
diff --git a/.rubocop/metrics.yml b/.rubocop/metrics.yml
@@ -32,3 +32,5 @@ Metrics/ModuleLength:
     - array
     - heredoc
     - method_call
+  Exclude:
+    - 'test/support/**/*.rb'
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,12 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Pattern matching support (`deconstruct_keys`) for Response, Response::Status,
   Headers, ContentType, and URI (#642)
+- Combined global and per-operation timeouts: global and per-operation timeouts
+  are no longer mutually exclusive. Use
+  `HTTP.timeout(global: 60, read: 30, write: 30, connect: 5)` to set both a
+  global request timeout and individual operation limits (#773)
 
 ### Changed
 
 - **BREAKING** Stricter timeout options parsing: `.timeout()` with a Hash now
-  rejects unknown keys, non-numeric values, string keys, and empty hashes.
-  `HTTP.timeout(global: N)` is no longer accepted; use `HTTP.timeout(N)` (#754)
+  rejects unknown keys, non-numeric values, string keys, and empty hashes (#754)
 - Bumped min llhttp dependency version
 - **BREAKING** Handle responses in the reverse order from the requests (#776)
 
diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb
@@ -161,6 +161,7 @@ def build_request(verb, uri, opts = {})
     #   @option options [Float] :read Read timeout
     #   @option options [Float] :write Write timeout
     #   @option options [Float] :connect Connect timeout
+    #   @option options [Float] :global Global timeout (combines with per-operation)
     # @overload timeout(global_timeout)
     #   Adds a global timeout to the full request
     #   @param [Numeric] global_timeout
@@ -169,9 +170,8 @@ def build_request(verb, uri, opts = {})
     def timeout(options)
       klass, options = case options
                        when Numeric then [HTTP::Timeout::Global, { global_timeout: options }]
-                       when Hash
-                         [HTTP::Timeout::PerOperation, HTTP::Timeout::PerOperation.normalize_options(options)]
-                       when :null then [HTTP::Timeout::Null, {}]
+                       when Hash    then resolve_timeout_hash(options)
+                       when :null   then [HTTP::Timeout::Null, {}]
                        else raise ArgumentError,
                                   "Use `.timeout(:null)`, " \
                                   "`.timeout(global_timeout_in_seconds)` or " \
diff --git a/lib/http/chainable/helpers.rb b/lib/http/chainable/helpers.rb
@@ -27,5 +27,36 @@ def build_proxy_hash(proxy)
       end
       result
     end
+
+    # Resolve a timeout hash into a timeout class and normalized options
+    #
+    # @example
+    #   resolve_timeout_hash(global: 60, read: 30)
+    #
+    # @param [Hash] options timeout options
+    # @return [Array(Class, Hash)] timeout class and normalized options
+    # @raise [ArgumentError] if options are invalid
+    # @api private
+    def resolve_timeout_hash(options)
+      remaining = options.dup
+      global = HTTP::Timeout::PerOperation.send(:extract_global_timeout!, remaining)
+
+      return resolve_global_only(global) if remaining.empty?
+
+      per_op = HTTP::Timeout::PerOperation.normalize_options(remaining)
+      global ? [HTTP::Timeout::Global, per_op.merge(global_timeout: global)] : [HTTP::Timeout::PerOperation, per_op]
+    end
+
+    # Build options for a global-only timeout from a hash
+    #
+    # @param [Numeric, nil] global the global timeout value
+    # @return [Array(Class, Hash)] timeout class and options
+    # @raise [ArgumentError] if no global timeout given
+    # @api private
+    def resolve_global_only(global)
+      raise ArgumentError, "no timeout options given" unless global
+
+      [HTTP::Timeout::Global, { global_timeout: global }]
+    end
   end
 end
diff --git a/lib/http/timeout/global.rb b/lib/http/timeout/global.rb
@@ -23,6 +23,9 @@ def initialize(*args)
         super
 
         @timeout = @time_left = options.fetch(:global_timeout)
+        @read_timeout    = options[:read_timeout]
+        @write_timeout   = options[:write_timeout]
+        @connect_timeout = options[:connect_timeout]
       end
 
       # Resets the time left counter to initial timeout
@@ -49,7 +52,7 @@ def reset_counter
       # @return [void]
       def connect(socket_class, host, port, nodelay: false)
         reset_timer
-        ::Timeout.timeout(@time_left, ConnectTimeoutError) do
+        ::Timeout.timeout(effective_timeout(@connect_timeout), ConnectTimeoutError) do
           @socket = socket_class.open(host, port)
           @socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) if nodelay
         end
@@ -70,10 +73,10 @@ def connect_ssl
         begin
           @socket.connect_nonblock
         rescue IO::WaitReadable
-          wait_readable_or_timeout
+          wait_readable_or_timeout(@connect_timeout)
           retry
         rescue IO::WaitWritable
-          wait_writable_or_timeout
+          wait_writable_or_timeout(@connect_timeout)
           retry
         end
       end
@@ -88,7 +91,7 @@ def connect_ssl
       # @api public
       # @return [String, :eof]
       def readpartial(size, buffer = nil)
-        perform_io { read_nonblock(size, buffer) }
+        perform_io(@read_timeout) { read_nonblock(size, buffer) }
       end
 
       # Write to the socket
@@ -100,7 +103,7 @@ def readpartial(size, buffer = nil)
       # @api public
       # @return [Integer, :eof]
       def write(data)
-        perform_io { write_nonblock(data) }
+        perform_io(@write_timeout) { write_nonblock(data) }
       end
 
       alias << write
@@ -125,18 +128,19 @@ def write_nonblock(data)
 
       # Performs I/O operation with timeout tracking
       #
+      # @param [Numeric, nil] per_op_timeout per-operation timeout limit
       # @api private
       # @return [Object]
-      def perform_io
+      def perform_io(per_op_timeout = nil)
         reset_timer
 
         loop do
           result = yield
           return handle_io_result(result) unless WAIT_RESULTS.include?(result)
 
-          wait_for_io(result)
-        rescue IO::WaitReadable then wait_readable_or_timeout
-        rescue IO::WaitWritable then wait_writable_or_timeout
+          wait_for_io(result, per_op_timeout)
+        rescue IO::WaitReadable then wait_readable_or_timeout(per_op_timeout)
+        rescue IO::WaitWritable then wait_writable_or_timeout(per_op_timeout)
         end
       rescue EOFError
         :eof
@@ -152,32 +156,53 @@ def handle_io_result(result)
 
       # Waits for an I/O readiness based on the result type
       #
+      # @param [Symbol] result the I/O wait type
+      # @param [Numeric, nil] per_op_timeout per-operation timeout limit
       # @api private
       # @return [void]
-      def wait_for_io(result)
+      def wait_for_io(result, per_op_timeout = nil)
         if result == :wait_readable
-          wait_readable_or_timeout
+          wait_readable_or_timeout(per_op_timeout)
         else
-          wait_writable_or_timeout
+          wait_writable_or_timeout(per_op_timeout)
         end
       end
 
       # Waits for a socket to become readable
       #
+      # @param [Numeric, nil] per_op per-operation timeout limit
       # @api private
       # @return [void]
-      def wait_readable_or_timeout
-        @socket.to_io.wait_readable(@time_left)
+      def wait_readable_or_timeout(per_op = nil)
+        timeout = effective_timeout(per_op)
+        result = @socket.to_io.wait_readable(timeout)
         log_time
+
+        raise TimeoutError, "Read timed out after #{per_op} seconds" if per_op && result.nil?
       end
 
       # Waits for a socket to become writable
       #
+      # @param [Numeric, nil] per_op per-operation timeout limit
       # @api private
       # @return [void]
-      def wait_writable_or_timeout
-        @socket.to_io.wait_writable(@time_left)
+      def wait_writable_or_timeout(per_op = nil)
+        timeout = effective_timeout(per_op)
+        result = @socket.to_io.wait_writable(timeout)
         log_time
+
+        raise TimeoutError, "Write timed out after #{per_op} seconds" if per_op && result.nil?
+      end
+
+      # Computes the effective timeout as the minimum of global and per-operation
+      #
+      # @param [Numeric, nil] per_op_timeout per-operation timeout limit
+      # @api private
+      # @return [Numeric]
+      def effective_timeout(per_op_timeout)
+        return @time_left unless per_op_timeout
+
+        [per_op_timeout, @time_left].min
       end
 
       # Resets the I/O timer to current time
diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb
@@ -43,6 +43,21 @@ def self.normalize_options(options)
         normalized
       end
 
+      # Extract and validate global timeout from options hash
+      #
+      # @example
+      #   extract_global_timeout!({global: 60, read: 5})
+      #
+      # @param [Hash] options mutable options hash (global key is deleted if found)
+      # @return [Numeric, nil] the global timeout value, or nil if not present
+      # @raise [ArgumentError] if both forms given or value is not numeric
+      # @api private
+      private_class_method def self.extract_global_timeout!(options)
+        return unless options.key?(:global) || options.key?(:global_timeout)
+
+        resolve_timeout_value!(options, :global, :global_timeout)
+      end
+
       # Resolve a single timeout value from the options hash
       #
       # @example
diff --git a/sig/http.rbs b/sig/http.rbs
@@ -50,6 +50,8 @@ module HTTP
 
     def branch: (Options options) -> Client
     def build_proxy_hash: (Array[untyped] proxy) -> Hash[Symbol, untyped]
+    def resolve_timeout_hash: (Hash[Symbol, untyped] options) -> [Class, Hash[Symbol, untyped]]
+    def resolve_global_only: (Numeric? global) -> [Class, Hash[Symbol, untyped]]
   end
 
   class Client
@@ -980,6 +982,7 @@ module HTTP
 
       private
 
+      def self.extract_global_timeout!: (Hash[Symbol, untyped] options) -> Numeric?
       def self.resolve_timeout_value!: (Hash[Symbol, untyped] options, Symbol short, Symbol long) -> Numeric
 
       attr_reader read_timeout: untyped
@@ -998,10 +1001,9 @@ module HTTP
       @timeout: Numeric
       @time_left: Numeric
       @started: Time
-
-      attr_reader read_timeout: untyped
-      attr_reader write_timeout: untyped
-      attr_reader connect_timeout: untyped
+      @read_timeout: Numeric?
+      @write_timeout: Numeric?
+      @connect_timeout: Numeric?
 
       def initialize: (*untyped args) -> void
       def reset_counter: () -> Numeric
@@ -1013,11 +1015,12 @@ module HTTP
 
       def read_nonblock: (Integer size, ?String? buffer) -> untyped
       def write_nonblock: (String data) -> untyped
-      def perform_io: () { () -> untyped } -> untyped
+      def perform_io: (?Numeric? per_op_timeout) { () -> untyped } -> untyped
       def handle_io_result: (untyped result) -> untyped
-      def wait_for_io: (untyped result) -> void
-      def wait_readable_or_timeout: () -> void
-      def wait_writable_or_timeout: () -> void
+      def wait_for_io: (untyped result, ?Numeric? per_op_timeout) -> void
+      def wait_readable_or_timeout: (?Numeric? per_op) -> void
+      def wait_writable_or_timeout: (?Numeric? per_op) -> void
+      def effective_timeout: (Numeric? per_op_timeout) -> Numeric
       def reset_timer: () -> void
       def log_time: () -> void
     end
diff --git a/test/http/timeout/global_test.rb b/test/http/timeout/global_test.rb
@@ -183,4 +183,91 @@
       end
     end
   end
+
+  context "with per-operation timeouts" do
+    let(:timeout) do
+      HTTP::Timeout::Global.new(global_timeout: 100, read_timeout: 100, write_timeout: 100, connect_timeout: 100)
+    end
+
+    describe "#readpartial when global is shorter than per-op" do
+      it "uses global time_left as effective timeout" do
+        call_count = 0
+        socket = fake(
+          to_io:         io,
+          closed?:       false,
+          read_nonblock: proc { |*|
+            call_count += 1
+            call_count == 1 ? :wait_readable : "data"
+          }
+        )
+        timeout.instance_variable_set(:@socket, socket)
+
+        assert_equal "data", timeout.readpartial(10)
+      end
+    end
+
+    context "with tight per-op timeouts" do
+      let(:timeout) do
+        HTTP::Timeout::Global.new(global_timeout: 100, read_timeout: 0.01, write_timeout: 0.01, connect_timeout: 0.01)
+      end
+
+      describe "#readpartial" do
+        it "raises when per-op read timeout fires before global" do
+          io_nil = fake(wait_readable: nil, wait_writable: true)
+          socket = fake(
+            to_io:         io_nil,
+            closed?:       false,
+            read_nonblock: :wait_readable
+          )
+          timeout.instance_variable_set(:@socket, socket)
+
+          err = assert_raises(HTTP::TimeoutError) { timeout.readpartial(10) }
+
+          assert_match(/Read timed out/, err.message)
+        end
+      end
+
+      describe "#write" do
+        it "raises when per-op write timeout fires before global" do
+          io_nil = fake(wait_readable: true, wait_writable: nil)
+          socket = fake(
+            to_io:          io_nil,
+            closed?:        false,
+            write_nonblock: :wait_writable
+          )
+          timeout.instance_variable_set(:@socket, socket)
+
+          err = assert_raises(HTTP::TimeoutError) { timeout.write("data") }
+
+          assert_match(/Write timed out/, err.message)
+        end
+      end
+
+      describe "#connect_ssl" do
+        it "uses connect_timeout for SSL handshake wait_readable" do
+          io_nil = fake(wait_readable: nil, wait_writable: true)
+          socket = fake(
+            to_io:            io_nil,
+            closed?:          false,
+            connect_nonblock: ->(*) { raise IO::EAGAINWaitReadable }
+          )
+          timeout.instance_variable_set(:@socket, socket)
+
+          assert_raises(HTTP::TimeoutError) { timeout.connect_ssl }
+        end
+
+        it "uses connect_timeout for SSL handshake wait_writable" do
+          io_nil = fake(wait_readable: true, wait_writable: nil)
+          socket = fake(
+            to_io:            io_nil,
+            closed?:          false,
+            connect_nonblock: ->(*) { raise IO::EAGAINWaitWritable }
+          )
+          timeout.instance_variable_set(:@socket, socket)
+
+          assert_raises(HTTP::TimeoutError) { timeout.connect_ssl }
+        end
+      end
+    end
+  end
 end
diff --git a/test/http/timeout/per_operation_test.rb b/test/http/timeout/per_operation_test.rb
diff --git a/test/http_test.rb b/test/http_test.rb
diff --git a/test/support/http_handling_shared/timeout_tests.rb b/test/support/http_handling_shared/timeout_tests.rb
