Merge pull request #340 from mauricio/issue-298

Retry only HTTP methods known to be idempotent

User can optionally configure other methods.

Fixes #298

lib/faraday/request/retry.rb

@@ -19,7 +19,12 @@ module Faraday
   # interval that is random between 0.1 and 0.15
   #
   class Request::Retry < Faraday::Middleware
-    class Options < Faraday::Options.new(:max, :interval, :interval_randomness, :backoff_factor, :exceptions)
+
+    IDEMPOTENT_METHODS = [:delete, :get, :head, :options, :put]
+
+    class Options < Faraday::Options.new(:max, :interval, :interval_randomness, :backoff_factor, :exceptions, :retry_if)
+      DEFAULT_CHECK = lambda { |env,exception| false }
+
       def self.from(value)
         if Fixnum === value
           new(value)
@@ -49,6 +54,10 @@ module Faraday
                                      Error::TimeoutError])
       end
 
+      def retry_if
+        self[:retry_if] ||= DEFAULT_CHECK
+      end
+
     end
 
     # Public: Initialize middleware
@@ -66,6 +75,12 @@ module Faraday
     #                       given as Class, Module, or String. (default:
     #                       [Errno::ETIMEDOUT, Timeout::Error,
     #                       Error::TimeoutError])
+    # retry_if            - block that will receive the env object and the exception raised
+    #                       and should decide if the code should retry still the action or
+    #                       not independent of the retry count. This would be useful
+    #                       if the exception produced is non-recoverable or if the
+    #                       the HTTP method called is not idempotent.
+    #                       (defaults to return false)
     def initialize(app, options = nil)
       super(app)
       @options = Options.from(options)
@@ -85,8 +100,8 @@ module Faraday
       begin
         env[:body] = request_body # after failure env[:body] is set to the response body
         @app.call(env)
-      rescue @errmatch
-        if retries > 0
+      rescue @errmatch => exception
+        if retries > 0 && retry_request?(env, exception)
           retries -= 1
           sleep sleep_amount(retries + 1)
           retry
@@ -114,5 +129,12 @@ module Faraday
       end
       matcher
     end
+
+    private
+
+    def retry_request?(env, exception)
+      IDEMPOTENT_METHODS.include?(env[:method]) || @options.retry_if.call(env, exception)
+    end
+
   end
 end

test/middleware/retry_test.rb

@@ -10,35 +10,37 @@ module Middleware
       Faraday.new do |b|
         b.request :retry, *retry_args
         b.adapter :test do |stub|
-          stub.post('/unstable') {
-            @times_called += 1
-            @explode.call @times_called
-          }
+          ['get', 'post'].each do |method|
+            stub.send(method, '/unstable') {
+              @times_called += 1
+              @explode.call @times_called
+            }
+          end
         end
       end
     end
 
     def test_unhandled_error
       @explode = lambda {|n| raise "boom!" }
-      assert_raises(RuntimeError) { conn.post("/unstable") }
+      assert_raises(RuntimeError) { conn.get("/unstable") }
       assert_equal 1, @times_called
     end
 
     def test_handled_error
       @explode = lambda {|n| raise Errno::ETIMEDOUT }
-      assert_raises(Errno::ETIMEDOUT) { conn.post("/unstable") }
+      assert_raises(Errno::ETIMEDOUT) { conn.get("/unstable") }
       assert_equal 3, @times_called
     end
 
     def test_legacy_max_retries
       @explode = lambda {|n| raise Errno::ETIMEDOUT }
-      assert_raises(Errno::ETIMEDOUT) { conn(1).post("/unstable") }
+      assert_raises(Errno::ETIMEDOUT) { conn(1).get("/unstable") }
       assert_equal 2, @times_called
     end
 
     def test_new_max_retries
       @explode = lambda {|n| raise Errno::ETIMEDOUT }
-      assert_raises(Errno::ETIMEDOUT) { conn(:max => 3).post("/unstable") }
+      assert_raises(Errno::ETIMEDOUT) { conn(:max => 3).get("/unstable") }
       assert_equal 4, @times_called
     end
 
@@ -46,7 +48,7 @@ module Middleware
       @explode = lambda {|n| raise Errno::ETIMEDOUT }
       started  = Time.now
       assert_raises(Errno::ETIMEDOUT) {
-        conn(:max => 2, :interval => 0.1).post("/unstable")
+        conn(:max => 2, :interval => 0.1).get("/unstable")
       }
       assert_in_delta 0.2, Time.now - started, 0.04
     end
@@ -70,7 +72,7 @@ module Middleware
       retry_middleware.sleep_amount_retries = [2, 1]
 
       assert_raises(Errno::ETIMEDOUT) {
-        retry_middleware.call({})
+        retry_middleware.call({:method => :get})
       }
 
       assert_empty retry_middleware.sleep_amount_retries
@@ -101,9 +103,45 @@ module Middleware
     def test_custom_exceptions
       @explode = lambda {|n| raise "boom!" }
       assert_raises(RuntimeError) {
-        conn(:exceptions => StandardError).post("/unstable")
+        conn(:exceptions => StandardError).get("/unstable")
       }
       assert_equal 3, @times_called
     end
+
+    def test_should_stop_retrying_if_block_returns_false_checking_env
+      @explode = lambda {|n| raise Errno::ETIMEDOUT }
+      check = lambda { |env,exception| env[:method] != :post }
+      assert_raises(Errno::ETIMEDOUT) {
+        conn(:retry_if => check).post("/unstable")
+      }
+      assert_equal 1, @times_called
+    end
+
+    def test_should_stop_retrying_if_block_returns_false_checking_exception
+      @explode = lambda {|n| raise Errno::ETIMEDOUT }
+      check = lambda { |env,exception| !exception.kind_of?(Errno::ETIMEDOUT) }
+      assert_raises(Errno::ETIMEDOUT) {
+        conn(:retry_if => check).post("/unstable")
+      }
+      assert_equal 1, @times_called
+    end
+
+    def test_should_not_call_retry_if_for_idempotent_methods
+      @explode = lambda {|n| raise Errno::ETIMEDOUT }
+      check = lambda { |env,exception| raise "this should have never been called" }
+      assert_raises(Errno::ETIMEDOUT) {
+        conn(:retry_if => check).get("/unstable")
+      }
+      assert_equal 3, @times_called
+    end
+
+    def test_should_not_retry_for_non_idempotent_method
+      @explode = lambda {|n| raise Errno::ETIMEDOUT }
+      assert_raises(Errno::ETIMEDOUT) {
+        conn.post("/unstable")
+      }
+      assert_equal 1, @times_called
+    end
+
   end
 end