Retry for all error codes >= 500 when x-stripe-should-retry is not se…

…t to true (#1368)

lib/stripe/stripe_client.rb

@@ -110,7 +110,7 @@ def self.default_connection_manager(config = Stripe.config)
     # both socket errors that may represent an intermittent problem and some
     # special HTTP statuses.
     def self.should_retry?(error,
-                           method:, num_retries:, config: Stripe.config)
+                           num_retries:, config: Stripe.config)
       return false if num_retries >= config.max_network_retries
 
       case error
@@ -143,15 +143,12 @@ def self.should_retry?(error,
         # These 429s are safe to retry.
         return true if error.http_status == 429 && error.code == "lock_timeout"
 
-        # 500 Internal Server Error
+        # Retry on 500, 503, and other internal errors.
         #
-        # We only bother retrying these for non-POST requests. POSTs end up
-        # being cached by the idempotency layer so there's no purpose in
-        # retrying them.
-        return true if error.http_status == 500 && method != :post
-
-        # 503 Service Unavailable
-        error.http_status == 503
+        # Note that we expect the stripe-should-retry header to be false
+        # in most cases when a 500 is returned, since our idempotency framework
+        # would typically replay it anyway.
+        true if error.http_status >= 500
       else
         false
       end
@@ -490,7 +487,7 @@ def self.maybe_gc_connection_managers
         end
 
       http_resp =
-        execute_request_with_rescues(method, api_base, headers, usage, context) do
+        execute_request_with_rescues(api_base, headers, usage, context) do
           self.class
               .default_connection_manager(config)
               .execute_request(method, url,
@@ -560,7 +557,7 @@ def self.maybe_gc_connection_managers
       http_status >= 400
     end
 
-    private def execute_request_with_rescues(method, api_base, headers, usage, context)
+    private def execute_request_with_rescues(api_base, headers, usage, context)
       num_retries = 0
 
       begin
@@ -611,7 +608,6 @@ def self.maybe_gc_connection_managers
                            user_data, resp, headers)
 
         if self.class.should_retry?(e,
-                                    method: method,
                                     num_retries: num_retries,
                                     config: config)
           num_retries += 1

test/stripe/stripe_client_test.rb

@@ -287,42 +287,42 @@ class StripeClientTest < Test::Unit::TestCase
 
       should "retry on Errno::ECONNREFUSED" do
         assert StripeClient.should_retry?(Errno::ECONNREFUSED.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on EOFError" do
         assert StripeClient.should_retry?(EOFError.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on Errno::ECONNRESET" do
         assert StripeClient.should_retry?(Errno::ECONNRESET.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on Errno::ETIMEDOUT" do
         assert StripeClient.should_retry?(Errno::ETIMEDOUT.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on Errno::EHOSTUNREACH" do
         assert StripeClient.should_retry?(Errno::EHOSTUNREACH.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on Net::OpenTimeout" do
         assert StripeClient.should_retry?(Net::OpenTimeout.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on Net::ReadTimeout" do
         assert StripeClient.should_retry?(Net::ReadTimeout.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on SocketError" do
         assert StripeClient.should_retry?(SocketError.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry when the `Stripe-Should-Retry` header is `true`" do
@@ -333,7 +333,7 @@ class StripeClientTest < Test::Unit::TestCase
         # Note we send status 400 here, which would normally not be retried.
         assert StripeClient.should_retry?(Stripe::StripeError.new(http_headers: headers,
                                                                   http_status: 400),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "not retry when the `Stripe-Should-Retry` header is `false`" do
@@ -344,49 +344,44 @@ class StripeClientTest < Test::Unit::TestCase
         # Note we send status 409 here, which would normally be retried.
         refute StripeClient.should_retry?(Stripe::StripeError.new(http_headers: headers,
                                                                   http_status: 409),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on a 409 Conflict" do
         assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 409),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on a 429 Too Many Requests when lock timeout" do
         assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 429,
                                                                   code: "lock_timeout"),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
-      should "retry on a 500 Internal Server Error when non-POST" do
+      should "retry on a 500 Internal Server Error" do
         assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 500),
-                                          method: :get, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "retry on a 503 Service Unavailable" do
         assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 503),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "not retry at maximum count" do
         refute StripeClient.should_retry?(RuntimeError.new,
-                                          method: :post, num_retries: Stripe.max_network_retries)
+                                          num_retries: Stripe.max_network_retries)
       end
 
       should "not retry on a certificate validation error" do
         refute StripeClient.should_retry?(OpenSSL::SSL::SSLError.new,
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
 
       should "not retry on a 429 Too Many Requests when not lock timeout" do
         refute StripeClient.should_retry?(Stripe::StripeError.new(http_status: 429,
                                                                   code: "rate_limited"),
-                                          method: :post, num_retries: 0)
-      end
-
-      should "not retry on a 500 Internal Server Error when POST" do
-        refute StripeClient.should_retry?(Stripe::StripeError.new(http_status: 500),
-                                          method: :post, num_retries: 0)
+                                          num_retries: 0)
       end
     end