Merge branch 'gh-82' into 'master'

persistent plugin: drop , allow retries for ping requests, regardless of idempotency property

See merge request os85/httpx!381

lib/httpx/connection.rb

@@ -538,10 +538,10 @@ module HTTPX
     def send_request_to_parser(request)
       @inflight += 1
       request.peer_address = @io.ip
-      parser.send(request)
-
       set_request_timeouts(request)
 
+      parser.send(request)
+
       return unless @state == :inactive
 
       transition(:active)

lib/httpx/connection/http2.rb

@@ -152,7 +152,7 @@ module HTTPX
 
     def ping
       ping = SecureRandom.gen_random(8)
-      @connection.ping(ping)
+      @connection.ping(ping.dup)
     ensure
       @pings << ping
     end

lib/httpx/plugins/persistent.rb

@@ -18,13 +18,25 @@ module HTTPX
     # https://gitlab.com/os85/httpx/wikis/Persistent
     #
     module Persistent
+      # subset of retryable errors which are safe to retry when reconnecting
+      RECONNECTABLE_ERRORS = [
+        IOError,
+        EOFError,
+        Errno::ECONNRESET,
+        Errno::ECONNABORTED,
+        Errno::EPIPE,
+        Errno::EINVAL,
+        Errno::ETIMEDOUT,
+        ConnectionError,
+      ].freeze
+
       def self.load_dependencies(klass)
         max_retries = if klass.default_options.respond_to?(:max_retries)
           [klass.default_options.max_retries, 1].max
         else
           1
         end
-        klass.plugin(:retries, max_retries: max_retries, retry_change_requests: true)
+        klass.plugin(:retries, max_retries: max_retries)
       end
 
       def self.extra_options(options)
@@ -34,6 +46,20 @@ module HTTPX
       module InstanceMethods
         private
 
+        def repeatable_request?(request, _)
+          super || begin
+            return false unless request.ping?
+
+            response = request.response
+
+            return false unless response && response.is_a?(ErrorResponse)
+
+            error = response.error
+
+            RECONNECTABLE_ERRORS.any? { |klass| error.is_a?(klass) }
+          end
+        end
+
         def get_current_selector
           super(&nil) || begin
             return unless block_given?

lib/httpx/plugins/query.rb

@@ -23,7 +23,7 @@ module HTTPX
         module InstanceMethods
           private
 
-          def __repeatable_request?(request, options)
+          def repeatable_request?(request, options)
             super || request.verb == "QUERY"
           end
         end

lib/httpx/plugins/retries.rb

@@ -88,6 +88,7 @@ module HTTPX
       end
 
       module InstanceMethods
+        # returns a `:retries` plugin enabled session with +n+ maximum retries per request setting.
         def max_retries(n)
           with(max_retries: n)
         end
@@ -99,16 +100,16 @@ module HTTPX
 
           if response &&
              request.retries.positive? &&
-             __repeatable_request?(request, options) &&
+             repeatable_request?(request, options) &&
              (
                (
-                 response.is_a?(ErrorResponse) && __retryable_error?(response.error)
+                 response.is_a?(ErrorResponse) && retryable_error?(response.error)
                ) ||
                (
                  options.retry_on && options.retry_on.call(response)
                )
              )
-            __try_partial_retry(request, response)
+            try_partial_retry(request, response)
             log { "failed to get response, #{request.retries} tries to go..." }
             request.retries -= 1 unless request.ping? # do not exhaust retries on connection liveness probes
             request.transition(:idle)
@@ -143,11 +144,13 @@ module HTTPX
           response
         end
 
-        def __repeatable_request?(request, options)
+        # returns whether +request+ can be retried.
+        def repeatable_request?(request, options)
           IDEMPOTENT_METHODS.include?(request.verb) || options.retry_change_requests
         end
 
-        def __retryable_error?(ex)
+        # returns whether the +ex+ exception happend for a retriable request.
+        def retryable_error?(ex)
           RETRYABLE_ERRORS.any? { |klass| ex.is_a?(klass) }
         end
 
@@ -156,11 +159,11 @@ module HTTPX
         end
 
         #
-        # Atttempt to set the request to perform a partial range request.
+        # Attempt to set the request to perform a partial range request.
         # This happens if the peer server accepts byte-range requests, and
         # the last response contains some body payload.
         #
-        def __try_partial_retry(request, response)
+        def try_partial_retry(request, response)
           response = response.response if response.is_a?(ErrorResponse)
 
           return unless response
@@ -181,10 +184,13 @@ module HTTPX
       end
 
       module RequestMethods
+        # number of retries left.
         attr_accessor :retries
 
+        # a response partially received before.
         attr_writer :partial_response
 
+        # initializes the request instance, sets the number of retries for the request.
         def initialize(*args)
           super
           @retries = @options.max_retries

sig/plugins/persistent.rbs

@@ -1,6 +1,8 @@
 module HTTPX
   module Plugins
     module Persistent
+      RECONNECTABLE_ERRORS: Array[singleton(StandardError)]
+
       def self.load_dependencies: (singleton(Session)) -> void
 
       def self.extra_options: (Options) -> (Options)

sig/plugins/retries.rbs

@@ -31,11 +31,11 @@ module HTTPX
 
         def fetch_response: (retriesRequest request, Selector selector, retriesOptions options) -> (retriesResponse | ErrorResponse)?
 
-        def __repeatable_request?: (retriesRequest request, retriesOptions options) -> boolish
+        def repeatable_request?: (retriesRequest request, retriesOptions options) -> boolish
 
-        def __retryable_error?: (_Exception error) -> bool
+        def retryable_error?: (_Exception error) -> bool
 
-        def __try_partial_retry: (retriesRequest request, (retriesResponse | ErrorResponse) response) -> void
+        def try_partial_retry: (retriesRequest request, (retriesResponse | ErrorResponse) response) -> void
 
       end
 

standalone_tests/close_on_fork_test.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require "oj"
+require "test_helper"
+
+class CloseOnForkTest < Minitest::Test
+  include HTTPHelpers
+  include HTTPX
+
+  def test_close_on_fork_after_fork_callback
+    skip("MRI feature") unless Process.respond_to?(:fork)
+    GC.start # cleanup instances created by other tests
+
+    http = HTTPX.plugin(SessionWithPool).with(persistent: true, close_on_fork: true)
+    uri = URI(build_uri("/get"))
+    response = http.get(uri)
+    verify_status(response, 200)
+
+    assert http.connections.size == 1
+    assert http.connections.count { |c| c.state == :closed }.zero?, "should have no closed connections"
+    HTTPX::Session.after_fork
+    assert http.connections.size == 1
+    assert http.connections.count { |c| c.state == :closed } == 1, "should have a closed connection"
+  end
+
+  def test_close_on_fork_automatic_after_fork_callback
+    skip("MRI 3.1 feature") unless Process.respond_to?(:_fork)
+    GC.start # cleanup instances created by other tests
+
+    http = HTTPX.plugin(SessionWithPool).with(persistent: true, close_on_fork: true)
+    uri = URI(build_uri("/get"))
+    response = http.get(uri)
+    verify_status(response, 200)
+
+    assert http.connections.size == 1
+    assert http.connections.count { |c| c.state == :closed }.zero?, "should have no closed connections"
+    pid = fork do
+      assert http.connections.count { |c| c.state == :closed } == 1, "should have no closed connections"
+      exit!(0)
+    end
+    assert http.connections.count { |c| c.state == :closed }.zero?, "should have no closed connections"
+    _, status = Process.waitpid2(pid)
+    assert_predicate(status, :success?)
+  end
+
+  private
+
+  def scheme
+    "https://"
+  end
+
+  def request(verb = "GET", uri = "http://google.com", **args)
+    Request.new(verb, uri, Options.new, **args)
+  end
+
+  def response(*args)
+    Response.new(*args)
+  end
+end

test/adapters/faraday_test.rb

@@ -246,7 +246,7 @@ class FaradayTest < Minitest::Test
     }
     session = nil
     faraday_conn = faraday_connection(request: { read_timeout: 2 }) do |http|
-      session = http.with(max_retries: 1, retry_on: check_error)
+      session = http.with(max_retries: 1, retry_on: check_error, retry_change_requests: true)
     end
     adapter_handler = faraday_conn.builder.handlers.last
     faraday_conn.builder.insert_before adapter_handler, Faraday::Multipart::Middleware

test/support/requests/plugins/persistent.rb

@@ -26,16 +26,47 @@ module Requests
         retry_persistent_session = HTTPX.plugin(:persistent).plugin(:retries, max_retries: 4)
         options = retry_persistent_session.send(:default_options)
         assert options.max_retries == 4
-        assert options.retry_change_requests
         assert options.persistent
 
         persistent_retry_session = HTTPX.plugin(:retries, max_retries: 4).plugin(:persistent)
         options = persistent_retry_session.send(:default_options)
         assert options.max_retries == 4
-        assert options.retry_change_requests
         assert options.persistent
       end
 
+      def test_plugin_persistent_does_not_retry_change_requests_on_timeouts
+        check_error = ->(response) { response.is_a?(HTTPX::ErrorResponse) || response.status == 405 }
+        persistent_session = HTTPX
+                             .plugin(RequestInspector)
+                             .plugin(:persistent, retry_on: check_error) # because CI
+                             .with(timeout: { request_timeout: 3 })
+
+        response = persistent_session.post(build_uri("/delay/10"), body: ["a" * 1024])
+        assert check_error[response]
+        assert persistent_session.calls.zero?, "expect request to be built 0 times (was #{persistent_session.calls})"
+      end
+
+      def test_plugin_persistent_does_not_retry_change_requests_on_keep_alive_interval_timeouts
+        start_test_servlet(KeepAlivePongThenTimeoutSocketServer) do |server|
+          check_error = ->(response) { response.is_a?(HTTPX::ErrorResponse) || response.status == 405 }
+          persistent_session = HTTPX
+                               .plugin(RequestInspector)
+                               .plugin(:persistent, retry_on: check_error)
+                               .with(
+                                 ssl: { verify_mode: OpenSSL::SSL::VERIFY_NONE },
+                                 timeout: { keep_alive_timeout: 1, request_timeout: 2 }
+                               )
+
+          response = persistent_session.post(server.origin, body: "test")
+          verify_status(response, 200)
+          assert persistent_session.calls.zero?, "expect request to be built 0 times (was #{persistent_session.calls})"
+          sleep(2)
+          response = persistent_session.post(server.origin, body: "test")
+          assert check_error[response]
+          assert persistent_session.calls == 1, "expect request to be built 1 time (was #{persistent_session.calls})"
+        end
+      end
+
       def test_persistent_retry_http2_goaway
         return unless origin.start_with?("https")
 

test/support/servlets/keep_alive.rb

@@ -52,8 +52,6 @@ class KeepAlivePongThenGoawayServer < TestHTTP2Server
 end
 
 class KeepAlivePongThenCloseSocketServer < TestHTTP2Server
-  attr_reader :pings, :pongs
-
   def initialize
     @sent = false
     super
@@ -71,3 +69,21 @@ class KeepAlivePongThenCloseSocketServer < TestHTTP2Server
     end
   end
 end
+
+class KeepAlivePongThenTimeoutSocketServer < TestHTTP2Server
+  def initialize(interval: 10, **args)
+    @interval = interval
+    super(**args)
+  end
+
+  private
+
+  def handle_connection(conn, sock)
+    super
+    conn.on(:stream) do |stream|
+      stream.on(:close) do
+        sleep(@interval)
+      end
+    end
+  end
+end