From 0c2fed71baea160918cf9028f9059e37e96f3932 Mon Sep 17 00:00:00 2001 From: Michael Elfassy Date: Thu, 3 Oct 2019 09:36:53 -0400 Subject: [PATCH 1/2] user-friendly messages for EOFError network errors --- lib/stripe/stripe_client.rb | 8 ++++++++ test/stripe/stripe_client_test.rb | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 6ab00f2db..3c4315653 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -88,7 +88,11 @@ def self.should_retry?(error, method:, num_retries:) # Destination refused the connection, the connection was reset, or a # variety of other connection failures. This could occur from a single # saturated server, so retry in case it's intermittent. + return true if error.is_a?(EOFError) return true if error.is_a?(Errno::ECONNREFUSED) + return true if error.is_a?(Errno::ECONNRESET) + return true if error.is_a?(Errno::EHOSTUNREACH) + return true if error.is_a?(Errno::ETIMEDOUT) return true if error.is_a?(SocketError) if error.is_a?(Stripe::StripeError) @@ -296,7 +300,11 @@ def execute_request(method, path, # The original error message is also appended onto the final exception for # full transparency. NETWORK_ERROR_MESSAGES_MAP = { + EOFError => ERROR_MESSAGE_CONNECTION, Errno::ECONNREFUSED => ERROR_MESSAGE_CONNECTION, + Errno::ECONNRESET => ERROR_MESSAGE_CONNECTION, + Errno::EHOSTUNREACH => ERROR_MESSAGE_CONNECTION, + Errno::ETIMEDOUT => ERROR_MESSAGE_TIMEOUT_CONNECT, SocketError => ERROR_MESSAGE_CONNECTION, Net::OpenTimeout => ERROR_MESSAGE_TIMEOUT_CONNECT, diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index f14115980..adf9f7cc9 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -172,6 +172,26 @@ class StripeClientTest < Test::Unit::TestCase method: :post, num_retries: 0) end + should "retry on EOFError" do + assert StripeClient.should_retry?(EOFError.new, + method: :post, num_retries: 0) + end + + should "retry on Errno::ECONNRESET" do + assert StripeClient.should_retry?(Errno::ECONNRESET.new, + method: :post, num_retries: 0) + end + + should "retry on Errno::ETIMEDOUT" do + assert StripeClient.should_retry?(Errno::ETIMEDOUT.new, + method: :post, num_retries: 0) + end + + should "retry on Errno::EHOSTUNREACH" do + assert StripeClient.should_retry?(Errno::EHOSTUNREACH.new, + method: :post, num_retries: 0) + end + should "retry on Net::OpenTimeout" do assert StripeClient.should_retry?(Net::OpenTimeout.new, method: :post, num_retries: 0) From ab2a7c9f5abca2a0e6c3d6ccd20bc21138d14e15 Mon Sep 17 00:00:00 2001 From: Michael Elfassy Date: Thu, 3 Oct 2019 12:36:00 -0400 Subject: [PATCH 2/2] refactor should_retry to reduce cyclomatic complexity --- lib/stripe/stripe_client.rb | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 3c4315653..8a684230c 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -81,21 +81,17 @@ def self.default_connection_manager 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). - return true if error.is_a?(Net::OpenTimeout) - return true if error.is_a?(Net::ReadTimeout) - - # Destination refused the connection, the connection was reset, or a - # variety of other connection failures. This could occur from a single - # saturated server, so retry in case it's intermittent. - return true if error.is_a?(EOFError) - return true if error.is_a?(Errno::ECONNREFUSED) - return true if error.is_a?(Errno::ECONNRESET) - return true if error.is_a?(Errno::EHOSTUNREACH) - return true if error.is_a?(Errno::ETIMEDOUT) - return true if error.is_a?(SocketError) - - if error.is_a?(Stripe::StripeError) + case error + when Net::OpenTimeout, Net::ReadTimeout + # Retry on timeout-related problems (either on open or read). + true + when EOFError, Errno::ECONNREFUSED, Errno::ECONNRESET, + Errno::EHOSTUNREACH, Errno::ETIMEDOUT, SocketError + # Destination refused the connection, the connection was reset, or a + # variety of other connection failures. This could occur from a single + # saturated server, so retry in case it's intermittent. + true + when Stripe::StripeError # The API may ask us not to retry (e.g. if doing so would be a no-op), # or advise us to retry (e.g. in cases of lock timeouts). Defer to # those instructions if given. @@ -123,10 +119,10 @@ def self.should_retry?(error, method:, num_retries:) return true if error.http_status == 500 && method != :post # 503 Service Unavailable - return true if error.http_status == 503 + error.http_status == 503 + else + false end - - false end def self.sleep_time(num_retries)