Tweak retry logic to be a little more like stripe-node

.rubocop.yml

@@ -19,16 +19,20 @@ Layout/IndentFirstHashElement:
 Layout/IndentHeredoc:
   Enabled: false
 
-Metrics/LineLength:
+Metrics/ClassLength:
   Exclude:
     - "test/**/*.rb"
+
+Metrics/LineLength:
+  Exclude:
     - "lib/stripe/resources/**/*.rb"
+    - "test/**/*.rb"
 
 Metrics/MethodLength:
   # There's ~2 long methods in `StripeClient`. If we want to truncate those a
   # little, we could move this to be closer to ~30 (but the default of 10 is
   # probably too short).
-  Max: 50
+  Max: 55
 
 Metrics/ModuleLength:
   Enabled: false

.rubocop_todo.yml

@@ -8,7 +8,7 @@
 
 # Offense count: 23
 Metrics/AbcSize:
-  Max: 48
+  Max: 51
 
 # Offense count: 33
 # Configuration parameters: CountComments, ExcludedMethods.

lib/stripe/stripe_client.rb

@@ -39,7 +39,7 @@ def self.default_connection_manager
     # Checks if an error is a problem that we should retry on. This includes
     # both socket errors that may represent an intermittent problem and some
     # special HTTP statuses.
-    def self.should_retry?(error, num_retries)
+    def self.should_retry?(error, method:, num_retries:)
       return false if num_retries >= Stripe.max_network_retries
 
       # Retry on timeout-related problems (either on open or read).
@@ -53,8 +53,18 @@ def self.should_retry?(error, num_retries)
       return true if error.is_a?(SocketError)
 
       if error.is_a?(Stripe::StripeError)
-        # 409 conflict
+        # 409 Conflict
         return true if error.http_status == 409
+
+        # 500 Internal Server Error
+        #
+        # 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
+        return true if error.http_status == 503
       end
 
       false
@@ -99,6 +109,11 @@ def request
 
     def execute_request(method, path,
                         api_base: nil, api_key: nil, headers: {}, params: {})
+      raise ArgumentError, "method should be a symbol" \
+        unless method.is_a?(Symbol)
+      raise ArgumentError, "path should be a string" \
+        unless path.is_a?(String)
+
       api_base ||= Stripe.api_base
       api_key ||= Stripe.api_key
       params = Util.objects_to_ids(params)
@@ -159,7 +174,7 @@ def execute_request(method, path,
       context.path            = path
       context.query_params    = query
 
-      http_resp = execute_request_with_rescues(api_base, context) do
+      http_resp = execute_request_with_rescues(method, api_base, context) do
         connection_manager.execute_request(method, url,
                                            body: body,
                                            headers: headers,
@@ -276,7 +291,7 @@ def execute_request(method, path,
       [body, body_log]
     end
 
-    private def execute_request_with_rescues(api_base, context)
+    private def execute_request_with_rescues(method, api_base, context)
       num_retries = 0
       begin
         request_start = Time.now
@@ -310,7 +325,7 @@ def execute_request(method, path,
           log_response_error(error_context, request_start, e)
         end
 
-        if self.class.should_retry?(e, num_retries)
+        if self.class.should_retry?(e, method: method, num_retries: num_retries)
           num_retries += 1
           sleep self.class.sleep_time(num_retries)
           retry

test/stripe/stripe_client_test.rb

@@ -54,31 +54,43 @@ class StripeClientTest < Test::Unit::TestCase
       end
 
       should "retry on Errno::ECONNREFUSED" do
-        assert StripeClient.should_retry?(Errno::ECONNREFUSED.new, 0)
+        assert StripeClient.should_retry?(Errno::ECONNREFUSED.new,
+                                          method: :post, num_retries: 0)
       end
 
       should "retry on Net::OpenTimeout" do
-        assert StripeClient.should_retry?(Net::OpenTimeout.new, 0)
+        assert StripeClient.should_retry?(Net::OpenTimeout.new,
+                                          method: :post, num_retries: 0)
       end
 
       should "retry on Net::ReadTimeout" do
-        assert StripeClient.should_retry?(Net::ReadTimeout.new, 0)
+        assert StripeClient.should_retry?(Net::ReadTimeout.new,
+                                          method: :post, num_retries: 0)
       end
 
       should "retry on SocketError" do
-        assert StripeClient.should_retry?(SocketError.new, 0)
+        assert StripeClient.should_retry?(SocketError.new,
+                                          method: :post, num_retries: 0)
       end
 
-      should "retry on a conflict" do
-        assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 409), 0)
+      should "retry on a 409 Conflict" do
+        assert StripeClient.should_retry?(Stripe::StripeError.new(http_status: 409),
+                                          method: :post, 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)
       end
 
       should "not retry at maximum count" do
-        refute StripeClient.should_retry?(RuntimeError.new, Stripe.max_network_retries)
+        refute StripeClient.should_retry?(RuntimeError.new,
+                                          method: :post, num_retries: Stripe.max_network_retries)
       end
 
       should "not retry on a certificate validation error" do
-        refute StripeClient.should_retry?(OpenSSL::SSL::SSLError.new, 0)
+        refute StripeClient.should_retry?(OpenSSL::SSL::SSLError.new,
+                                          method: :post, num_retries: 0)
       end
     end