Retry middleware improvements (honour Retry-After header, retry statu…

…ses) (#773)

lib/faraday/error.rb

@@ -55,8 +55,12 @@ def initialize(ex = nil)
   class SSLError < ClientError
   end
 
+  class RetriableResponse < ClientError; end
+
   [:ClientError, :ConnectionFailed, :ResourceNotFound,
-   :ParsingError, :TimeoutError, :SSLError].each do |const|
+   :ParsingError, :TimeoutError, :SSLError, :RetriableResponse].each do |const|
     Error.const_set(const, Faraday.const_get(const))
   end
+
+
 end

lib/faraday/request/retry.rb

@@ -23,7 +23,9 @@ class Request::Retry < Faraday::Middleware
     IDEMPOTENT_METHODS = [:delete, :get, :head, :options, :put]
 
     class Options < Faraday::Options.new(:max, :interval, :max_interval, :interval_randomness,
-                                         :backoff_factor, :exceptions, :methods, :retry_if, :retry_block)
+                                         :backoff_factor, :exceptions, :methods, :retry_if, :retry_block,
+                                         :retry_statuses)
+
       DEFAULT_CHECK = lambda { |env,exception| false }
 
       def self.from(value)
@@ -56,7 +58,8 @@ def backoff_factor
 
       def exceptions
         Array(self[:exceptions] ||= [Errno::ETIMEDOUT, 'Timeout::Error',
-                                     Error::TimeoutError])
+                                     Error::TimeoutError,
+                                     Faraday::Error::RetriableResponse])
       end
 
       def methods
@@ -71,6 +74,9 @@ def retry_block
         self[:retry_block] ||= Proc.new {}
       end
 
+      def retry_statuses
+        Array(self[:retry_statuses] ||= [])
+      end
     end
 
     # Public: Initialize middleware
@@ -106,29 +112,35 @@ def initialize(app, options = nil)
       @errmatch = build_exception_matcher(@options.exceptions)
     end
 
-    def sleep_amount(retries)
-      retry_index = @options.max - retries
-      current_interval = @options.interval * (@options.backoff_factor ** retry_index)
-      current_interval = [current_interval, @options.max_interval].min
-      random_interval  = rand * @options.interval_randomness.to_f * @options.interval
-      current_interval + random_interval
+    def calculate_sleep_amount(retries, env)
+      retry_after     = calculate_retry_after(env)
+      retry_interval  = calculate_retry_interval(retries)
+
+      return if retry_after && retry_after > @options.max_interval
+
+      retry_after && retry_after >= retry_interval ? retry_after : retry_interval
     end
 
     def call(env)
       retries = @options.max
       request_body = env[:body]
       begin
         env[:body] = request_body # after failure env[:body] is set to the response body
-        @app.call(env)
+        @app.call(env).tap do |resp|
+          raise Faraday::Error::RetriableResponse.new(nil, resp) if @options.retry_statuses.include?(resp.status)
+        end
       rescue @errmatch => exception
         if retries > 0 && retry_request?(env, exception)
           retries -= 1
           rewind_files(request_body)
           @options.retry_block.call(env, @options, retries, exception)
-          sleep sleep_amount(retries + 1)
-          retry
+          if (sleep_amount = calculate_sleep_amount(retries + 1, env))
+            sleep sleep_amount
+            retry
+          end
         end
-        raise
+
+        raise unless exception.is_a?(Faraday::Error::RetriableResponse)
       end
     end
 
@@ -167,5 +179,29 @@ def rewind_files(body)
       end
     end
 
+    # MDN spec for Retry-After header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
+    def calculate_retry_after(env)
+      response_headers = env[:response_headers]
+      return unless response_headers
+
+      retry_after_value = env[:response_headers]["Retry-After"]
+
+      # Try to parse date from the header value
+      begin
+        datetime = DateTime.rfc2822(retry_after_value)
+        datetime.to_time - Time.now.utc
+      rescue ArgumentError
+        retry_after_value.to_f
+      end
+    end
+
+    def calculate_retry_interval(retries)
+      retry_index = @options.max - retries
+      current_interval = @options.interval * (@options.backoff_factor ** retry_index)
+      current_interval = [current_interval, @options.max_interval].min
+      random_interval  = rand * @options.interval_randomness.to_f * @options.interval
+
+      current_interval + random_interval
+    end
   end
 end

test/middleware/retry_test.rb

@@ -10,6 +10,7 @@ def setup
     def conn(*retry_args)
       Faraday.new do |b|
         b.request :retry, *retry_args
+
         b.adapter :test do |stub|
           ['get', 'post'].each do |method|
             stub.send(method, '/unstable') do |env|
@@ -18,6 +19,22 @@ def conn(*retry_args)
               env[:body] = nil # simulate blanking out response body
               @explode.call @times_called
             end
+
+            stub.send(method, '/throttled') do |env|
+              @times_called += 1
+              @envs << env.dup
+
+              params = env[:params]
+
+              status = (params['status'] || 429).to_i
+              headers = {}
+
+              retry_after = params['retry_after']
+
+              headers['Retry-After'] = retry_after if retry_after
+
+              [status, headers, '']
+            end
           end
         end
       end
@@ -68,7 +85,7 @@ def test_interval
       assert_in_delta 0.2, Time.now - started, 0.04
     end
 
-    def test_calls_sleep_amount
+    def test_calls_calculate_sleep_amount
       explode_app = MiniTest::Mock.new
       explode_app.expect(:call, nil, [{:body=>nil}])
       def explode_app.call(env)
@@ -79,7 +96,7 @@ def explode_app.call(env)
       class << retry_middleware
         attr_accessor :sleep_amount_retries
 
-        def sleep_amount(retries)
+        def calculate_sleep_amount(retries, env)
           self.sleep_amount_retries.delete(retries)
           0
         end
@@ -95,30 +112,30 @@ def sleep_amount(retries)
 
     def test_exponential_backoff
       middleware = Faraday::Request::Retry.new(nil, :max => 5, :interval => 0.1, :backoff_factor => 2)
-      assert_equal middleware.sleep_amount(5), 0.1
-      assert_equal middleware.sleep_amount(4), 0.2
-      assert_equal middleware.sleep_amount(3), 0.4
+      assert_equal middleware.send(:calculate_retry_interval, 5), 0.1
+      assert_equal middleware.send(:calculate_retry_interval, 4), 0.2
+      assert_equal middleware.send(:calculate_retry_interval, 3), 0.4
     end
 
     def test_exponential_backoff_with_max_interval
       middleware = Faraday::Request::Retry.new(nil, :max => 5, :interval => 1, :max_interval => 3, :backoff_factor => 2)
-      assert_equal middleware.sleep_amount(5), 1
-      assert_equal middleware.sleep_amount(4), 2
-      assert_equal middleware.sleep_amount(3), 3
-      assert_equal middleware.sleep_amount(2), 3
+      assert_equal middleware.send(:calculate_retry_interval, 5), 1
+      assert_equal middleware.send(:calculate_retry_interval, 4), 2
+      assert_equal middleware.send(:calculate_retry_interval, 3), 3
+      assert_equal middleware.send(:calculate_retry_interval, 2), 3
     end
 
     def test_random_additional_interval_amount
       middleware = Faraday::Request::Retry.new(nil, :max => 2, :interval => 0.1, :interval_randomness => 1.0)
-      sleep_amount = middleware.sleep_amount(2)
+      sleep_amount = middleware.send(:calculate_retry_interval, 2)
       assert_operator sleep_amount, :>=, 0.1
       assert_operator sleep_amount, :<=, 0.2
       middleware = Faraday::Request::Retry.new(nil, :max => 2, :interval => 0.1, :interval_randomness => 0.5)
-      sleep_amount = middleware.sleep_amount(2)
+      sleep_amount = middleware.send(:calculate_retry_interval, 2)
       assert_operator sleep_amount, :>=, 0.1
       assert_operator sleep_amount, :<=, 0.15
       middleware = Faraday::Request::Retry.new(nil, :max => 2, :interval => 0.1, :interval_randomness => 0.25)
-      sleep_amount = middleware.sleep_amount(2)
+      sleep_amount = middleware.send(:calculate_retry_interval, 2)
       assert_operator sleep_amount, :>=, 0.1
       assert_operator sleep_amount, :<=, 0.125
     end
@@ -212,5 +229,53 @@ def test_should_rewind_files_on_retry
       assert_equal 3, @times_called
       assert_equal 2, rewound
     end
+
+    def test_should_retry_retriable_response
+      params = { status: 429 }
+      conn(:max => 1, :retry_statuses => 429).get("/throttled", params)
+
+      assert_equal 2, @times_called
+    end
+
+    def test_should_not_retry_non_retriable_response
+      params = { status: 503 }
+      conn(:max => 1, :retry_statuses => 429).get("/throttled", params)
+
+      assert_equal 1, @times_called
+    end
+
+    def test_interval_if_retry_after_present
+      started = Time.now
+
+      params = { :retry_after => 0.5 }
+      conn(:max => 1, :interval => 0.1, :retry_statuses => [429]).get("/throttled", params)
+
+      assert Time.now - started > 0.5
+    end
+
+    def test_should_ignore_retry_after_if_less_then_calculated
+      started = Time.now
+
+      params = { :retry_after => 0.1 }
+      conn(:max => 1, :interval => 0.2, :retry_statuses => [429]).get("/throttled", params)
+
+      assert Time.now - started > 0.2
+    end
+
+    def test_interval_when_retry_after_is_timestamp
+      started = Time.now
+
+      params = { :retry_after => (Time.now.utc + 2).strftime('%a, %d %b %Y %H:%M:%S GMT') }
+      conn(:max => 1, :interval => 0.1, :retry_statuses => [429]).get("/throttled", params)
+
+      assert Time.now - started > 1
+    end
+
+    def test_should_not_retry_when_retry_after_greater_then_max_interval
+      params = { :retry_after => (Time.now.utc + 20).strftime('%a, %d %b %Y %H:%M:%S GMT') }
+      conn(:max => 2, :interval => 0.1, :retry_statuses => [429], max_interval: 5).get("/throttled", params)
+
+      assert_equal 1, @times_called
+    end
   end
 end