Resend idempotent requests when a reused connection was closed

CHANGELOG.md

@@ -7,8 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- `HTTP::Request#replayable?` reports whether a request can be sent again
+  after a connection failure: its method is idempotent (RFC 9110 Section 9.2.2)
+  or it carries an `Idempotency-Key` / `X-Idempotency-Key` header, and its body
+  is nil or a String.
+
 ### Fixed
 
+- Persistent clients now resend a replayable request once, on a new
+  connection, when a reused connection fails before any response byte arrives,
+  typically because the server or a proxy closed it while it sat idle.
+  Previously the request raised `HTTP::ResponseHeaderError` ("couldn't read
+  response headers"), `HTTP::SocketReadError`, `HTTP::SocketWriteError` or,
+  over TLS on OpenSSL 3, `OpenSSL::SSL::SSLError`. Non-replayable requests,
+  clients with a `retriable` policy, timeouts, and failures after part of the
+  response arrived still raise. ([#420], [#459])
 - Building a default `Host` header now raises `HTTP::RequestError` when the
   request URI has a nil host (previously `NoMethodError`) or an empty host
   (e.g. `https:///path` or `https://:123/path`, which previously produced
@@ -298,9 +313,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#358]: https://github.com/httprb/http/issues/358
 [#371]: https://github.com/httprb/http/issues/371
 [#372]: https://github.com/httprb/http/issues/372
+[#420]: https://github.com/httprb/http/issues/420
 [#447]: https://github.com/httprb/http/issues/447
 [#448]: https://github.com/httprb/http/issues/448
 [#449]: https://github.com/httprb/http/issues/449
+[#459]: https://github.com/httprb/http/issues/459
 [#491]: https://github.com/httprb/http/issues/491
 [#493]: https://github.com/httprb/http/pull/493
 [#512]: https://github.com/httprb/http/issues/512

lib/http/client.rb

@@ -1,12 +1,14 @@
 # frozen_string_literal: true
 
 require "forwardable"
+require "openssl"
 
 require "http/form_data"
 require "http/retriable/performer"
 require "http/options"
 require "http/feature"
 require "http/headers"
+require "http/client/connection_reuse"
 require "http/connection"
 require "http/redirector"
 require "http/request/builder"
@@ -17,6 +19,7 @@ module HTTP
   class Client
     extend Forwardable
     include Chainable
+    include ConnectionReuse
 
     # Initialize a new HTTP Client
     #
@@ -142,14 +145,8 @@ def perform_with_retry(req, options)
     # @return [void]
     # @api private
     def send_request(req, options)
-      notify_features(req, options)
-
-      @connection ||= HTTP::Connection.new(req, options)
-
-      unless @connection.failed_proxy_connect?
-        @connection.send_request(req)
-        @connection.read_headers!
-      end
+      options.features.each_value { |feature| feature.on_request(req) }
+      transmit(req, options)
     rescue Error => e
       options.features.each_value { |feature| feature.on_error(req, e) }
       raise
@@ -166,13 +163,6 @@ def build_wrapped_response(req, options)
       end
     end
 
-    # Notify features of an upcoming request attempt
-    # @return [void]
-    # @api private
-    def notify_features(req, options)
-      options.features.each_value { |feature| feature.on_request(req) }
-    end
-
     # Execute the HTTP exchange wrapped by feature around_request hooks
     # @return [HTTP::Response] the response
     # @api private

lib/http/client/connection_reuse.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module HTTP
+  class Client
+    # Sends requests over the client's connection, once more on a new
+    # connection when a reused one turns out to be closed
+    module ConnectionReuse
+      private
+
+      # Write the request and read the response headers
+      #
+      # A reused connection may have been closed by the peer while it sat
+      # idle. When that surfaces before any response byte arrives, a
+      # replayable request is sent once more on a fresh connection. The resend
+      # always starts from a new connection, so it happens at most once.
+      #
+      # @return [void]
+      # @api private
+      def transmit(req, options)
+        reused = !@connection.nil?
+        @connection ||= Connection.new(req, options)
+        return if @connection.failed_proxy_connect?
+
+        @connection.send_request(req)
+        @connection.read_headers!
+      rescue ConnectionError, OpenSSL::SSL::SSLError
+        raise unless reused && resend?(req, options)
+
+        @connection.close
+        @connection = nil
+        retry
+      end
+
+      # Whether a request that failed on a reused connection can be resent
+      #
+      # Explicit retry policies set with {Chainable#retriable} take precedence.
+      #
+      # @return [Boolean]
+      # @api private
+      def resend?(req, options)
+        !options.retriable && req.replayable? && !@connection.response_started?
+      end
+    end
+  end
+end

lib/http/connection.rb

@@ -107,7 +107,8 @@ def send_request(req)
         raise StateError, "Tried to send a request while a response is pending. Make sure you read off the body."
       end
 
-      @pending_request = true
+      @response_started = false
+      @pending_request  = true
 
       req.stream @socket
 
@@ -199,6 +200,17 @@ def finished_request?
       !@pending_request && !@pending_response
     end
 
+    # Whether response bytes have arrived since the last request was sent
+    #
+    # @example
+    #   connection.response_started? # => false
+    #
+    # @return [Boolean]
+    # @api public
+    def response_started?
+      @response_started
+    end
+
     # Whether we're keeping the conn alive
     #
     # @example
@@ -231,25 +243,12 @@ def init_state(options)
       @keep_alive_timeout   = options.keep_alive_timeout.to_f
       @pending_request      = false
       @pending_response     = false
+      @response_started     = false
       @failed_proxy_connect = false
       @buffer               = "".b
       @parser               = Response::Parser.new
     end
 
-    # Check for premature end-of-file and raise if detected
-    #
-    # @example
-    #   check_premature_eof(:eof)
-    #
-    # @return [void]
-    # @api private
-    def check_premature_eof(eof)
-      return unless eof && !@parser.finished? && body_framed?
-
-      close
-      raise ConnectionError, "response body ended prematurely"
-    end
-
     # Connect socket and set up proxy/TLS
     # @return [void]
     # @api private

lib/http/connection/internals.rb

@@ -107,6 +107,20 @@ def set_keep_alive
           end
       end
 
+      # Check for premature end-of-file and raise if detected
+      #
+      # @example
+      #   check_premature_eof(:eof)
+      #
+      # @return [void]
+      # @api private
+      def check_premature_eof(eof)
+        return unless eof && !@parser.finished? && body_framed?
+
+        close
+        raise ConnectionError, "response body ended prematurely"
+      end
+
       # Check if the response body has a known framing mechanism
       #
       # @example
@@ -131,11 +145,25 @@ def read_more(size)
           @parser << ""
           :eof
         elsif value
-          @parser << value
+          feed_parser(value)
         end
       rescue IOError, SocketError, SystemCallError => e
         raise SocketReadError, "error reading from socket: #{e}", e.backtrace
       end
+
+      # Marks the response as started, then feeds a chunk into parser
+      #
+      # The mark comes first so a chunk the parser rejects still counts.
+      #
+      # @example
+      #   feed_parser("HTTP/1.1 200 OK\r\n")
+      #
+      # @return [Response::Parser]
+      # @api private
+      def feed_parser(chunk)
+        @response_started = true
+        @parser << chunk
+      end
     end
   end
 end

lib/http/request.rb

@@ -7,6 +7,7 @@
 require "http/errors"
 require "http/headers"
 require "http/request/body"
+require "http/request/idempotency"
 require "http/request/proxy"
 require "http/request/writer"
 require "http/version"
@@ -19,6 +20,7 @@ class Request
 
     include HTTP::Base64
     include Proxy
+    include Idempotency
 
     # The method given was not understood
     class UnsupportedMethodError < RequestError; end

lib/http/request/idempotency.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module HTTP
+  class Request
+    # Decides whether a request can be sent again on a new connection
+    module Idempotency
+      # Idempotent methods (RFC 9110, Section 9.2.2)
+      IDEMPOTENT_METHODS = %i[get head options trace put delete].freeze
+
+      # Headers that mark any request as idempotent (draft-ietf-httpapi-idempotency-key-header)
+      IDEMPOTENCY_KEY_HEADERS = %w[Idempotency-Key X-Idempotency-Key].freeze
+
+      # Whether the request can be sent again after a connection failure
+      #
+      # True when the method is idempotent or an idempotency key header is
+      # present, and the body is nil or a String, so it can be written again.
+      #
+      # @example
+      #   request.replayable? # => true
+      #
+      # @return [Boolean]
+      # @api public
+      def replayable?
+        source = body.source
+        return false unless source.nil? || source.is_a?(String)
+
+        IDEMPOTENT_METHODS.include?(verb) || IDEMPOTENCY_KEY_HEADERS.any? { |name| headers.include?(name) }
+      end
+    end
+  end
+end

sig/http.rbs

@@ -127,6 +127,7 @@ module HTTP
   class Client
     extend Forwardable
     include Chainable
+    include Client::ConnectionReuse
 
     @default_options: Options
     @connection: untyped
@@ -166,13 +167,23 @@ module HTTP
 
     def perform_once: (Request req, Options options) -> Response
     def perform_with_retry: (Request req, Options options) -> Response
-    def notify_features: (Request req, Options options) -> void
     def perform_exchange: (Request req, Options options) -> Response
     def around_request: (Request request, Options options) { (Request) -> Response } -> Response
     def build_response: (Request req, Options options) -> Response
     def build_wrapped_response: (Request req, Options options) -> Response
     def send_request: (Request req, Options options) -> void
+    def transmit: (Request req, Options options) -> void
+    def resend?: (Request req, Options options) -> bool
     def verify_connection!: (URI uri) -> void
+
+    module ConnectionReuse
+      @connection: untyped
+
+      private
+
+      def transmit: (Request req, Options options) -> void
+      def resend?: (Request req, Options options) -> bool
+    end
   end
 
   class Session
@@ -805,6 +816,7 @@ module HTTP
     extend Forwardable
     include Base64
     include Request::Proxy
+    include Request::Idempotency
 
     class Builder
       HTTP_OR_HTTPS_RE: Regexp
@@ -891,6 +903,19 @@ module HTTP
       def port: () -> untyped
     end
 
+    module Idempotency : _IdempotencyHost
+      interface _IdempotencyHost
+        def body: () -> Request::Body
+        def verb: () -> verb
+        def headers: () -> Headers
+      end
+
+      IDEMPOTENT_METHODS: Array[verb]
+      IDEMPOTENCY_KEY_HEADERS: Array[String]
+
+      def replayable?: () -> bool
+    end
+
     class Body
       @source: untyped
 
@@ -1127,6 +1152,7 @@ module HTTP
     @keep_alive_timeout: Float
     @pending_request: bool
     @pending_response: bool | Response
+    @response_started: bool
     @failed_proxy_connect: bool
     @buffer: String
     @parser: Response::Parser
@@ -1145,6 +1171,7 @@ module HTTP
     def finish_response: () -> void
     def close: () -> void
     def finished_request?: () -> bool
+    def response_started?: () -> bool
     def keep_alive?: () -> bool
     def expired?: () -> bool
     def status_code: () -> Integer?
@@ -1179,8 +1206,10 @@ module HTTP
       def handle_proxy_connect_response: () -> void
       def reset_timer: () -> void
       def set_keep_alive: () -> void
+      def check_premature_eof: (bool eof) -> void
       def body_framed?: () -> bool
       def read_more: (Integer size) -> untyped
+      def feed_parser: (String chunk) -> Response::Parser
     end
   end