diff --git a/lib/stripe.rb b/lib/stripe.rb index 2cde4f41..4730cb5d 100644 --- a/lib/stripe.rb +++ b/lib/stripe.rb @@ -68,6 +68,23 @@ require 'stripe/errors/rate_limit_error' module Stripe DEFAULT_CA_BUNDLE_PATH = File.dirname(__FILE__) + '/data/ca-certificates.crt' + # HTTP status exceptions which we'd like to retry. Most HTTP status + # exceptions are from a Stripe API server response, so we generally _don't_ + # retry because doing so would have the same result as the original request. + RETRY_HTTP_EXCEPTIONS = [ + # A server may respond with a 409 to indicate that there is a concurrent + # request executing with the same idempotency key. In the case that a + # request failed due to a connection problem and the client has retried too + # early, but the server is still executing the old request, we would like + # the client to continue retrying until getting a "real" response status + # back. + RestClient::Conflict, + + # Retry on timeout-related problems. This shouldn't be lumped in with HTTP + # exceptions, but with RestClient it is. + RestClient::RequestTimeout, + ].freeze + @api_base = 'https://api.stripe.com' @connect_base = 'https://connect.stripe.com' @uploads_base = 'https://uploads.stripe.com' @@ -194,24 +211,45 @@ module Stripe def self.execute_request_with_rescues(request_opts, api_base_url, retry_count = 0) begin response = execute_request(request_opts) - rescue SocketError => e - response = handle_restclient_error(e, request_opts, retry_count, api_base_url) - rescue NoMethodError => e - # Work around RestClient bug - if e.message =~ /\WRequestFailed\W/ - e = APIConnectionError.new('Unexpected HTTP response code') + + # We rescue all exceptions from a request so that we have an easy spot to + # implement our retry logic across the board. We'll re-raise if it's a type + # of exception that we didn't expect to handle. + rescue => e + if should_retry?(e, retry_count) + retry_count = retry_count + 1 + sleep sleep_time(retry_count) + retry + end + + case e + when SocketError response = handle_restclient_error(e, request_opts, retry_count, api_base_url) + + when NoMethodError + # Work around RestClient bug + if e.message =~ /\WRequestFailed\W/ + e = APIConnectionError.new('Unexpected HTTP response code') + response = handle_restclient_error(e, request_opts, retry_count, api_base_url) + else + raise + end + + when RestClient::ExceptionWithResponse + if e.response + handle_api_error(e.response) + else + response = handle_restclient_error(e, request_opts, retry_count, api_base_url) + end + + when RestClient::Exception, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError + response = handle_restclient_error(e, request_opts, retry_count, api_base_url) + + # Only handle errors when we know we can do so, and re-raise otherwise. + # This should be pretty infrequent. else raise end - rescue RestClient::ExceptionWithResponse => e - if e.response - handle_api_error(e.response) - else - response = handle_restclient_error(e, request_opts, retry_count, api_base_url) - end - rescue RestClient::Exception, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError => e - response = handle_restclient_error(e, request_opts, retry_count, api_base_url) end response @@ -367,13 +405,6 @@ module Stripe def self.handle_restclient_error(e, request_opts, retry_count, api_base_url=nil) - if should_retry?(e, retry_count) - retry_count = retry_count + 1 - sleep sleep_time(retry_count) - response = execute_request_with_rescues(request_opts, api_base_url, retry_count) - return response - end - api_base_url = @api_base unless api_base_url connection_message = "Please check your internet connection and try again. " \ "If this problem persists, you should check Stripe's service status at " \ @@ -419,7 +450,16 @@ module Stripe def self.should_retry?(e, retry_count) return false if retry_count >= self.max_network_retries + + # Certificate validation problem: do not retry. return false if e.is_a?(RestClient::SSLCertificateNotVerified) + + # Generally don't retry when we got a successful response back from the + # Stripe API server, but with some exceptions (for more details, see notes + # on RETRY_HTTP_EXCEPTIONS). + return false if e.is_a?(RestClient::ExceptionWithResponse) && + !RETRY_HTTP_EXCEPTIONS.any? { |klass| e.is_a?(klass) } + return true end diff --git a/test/stripe/api_resource_test.rb b/test/stripe/api_resource_test.rb index de72db51..17382c98 100644 --- a/test/stripe/api_resource_test.rb +++ b/test/stripe/api_resource_test.rb @@ -670,14 +670,14 @@ module Stripe end context "with retries" do - setup do Stripe.stubs(:max_network_retries).returns(2) end should 'retry failed network requests if specified and raise if error persists' do Stripe.expects(:sleep_time).at_least_once.returns(0) - @mock.expects(:post).times(3).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd').raises(Errno::ECONNREFUSED.new) + @mock.expects(:post).times(3).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd'). + raises(Errno::ECONNREFUSED.new) err = assert_raises Stripe::APIConnectionError do Stripe::Charge.create(:amount => 50, :currency => 'usd', :card => { :number => nil }) @@ -689,34 +689,15 @@ module Stripe Stripe.expects(:sleep_time).at_least_once.returns(0) response = make_response({"id" => "myid"}) err = Errno::ECONNREFUSED.new - @mock.expects(:post).times(2).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd').raises(err).then.returns(response) + @mock.expects(:post).times(2).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd'). + raises(Errno::ECONNREFUSED.new). + then. + returns(response) result = Stripe::Charge.create(:amount => 50, :currency => 'usd', :card => { :number => nil }) assert_equal "myid", result.id end - # We retry the request if we receive SSL errors, since these can be caused - # by transient network issues, in addition to compatibility issues between - # the client and server. - should 'retry failed network requests if they fail with OpenSSL::SSL::SSLError' do - Stripe.expects(:sleep_time).at_least_once.returns(0) - @mock.expects(:post).times(3).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd').raises(OpenSSL::SSL::SSLError.new('message')) - - err = assert_raises Stripe::APIConnectionError do - Stripe::Charge.create(:amount => 50, :currency => 'usd', :card => { :number => nil }) - end - assert_match(/Request was retried 2 times/, err.message) - end - - should 'not retry a SSLCertificateNotVerified error' do - @mock.expects(:post).times(1).with('https://api.stripe.com/v1/charges', nil, 'amount=50¤cy=usd').raises(RestClient::SSLCertificateNotVerified.new('message')) - - err = assert_raises Stripe::APIConnectionError do - Stripe::Charge.create(:amount => 50, :currency => 'usd', :card => { :number => nil }) - end - assert_no_match(/retried/, err.message) - end - should 'not add an idempotency key to GET requests' do SecureRandom.expects(:uuid).times(0) Stripe.expects(:execute_request).with do |opts| @@ -755,8 +736,33 @@ module Stripe end - context "sleep_time" do + context ".should_retry?" do + setup do + Stripe.stubs(:max_network_retries).returns(2) + end + should 'retry on a low-level network error' do + assert Stripe.should_retry?(Errno::ECONNREFUSED.new, 0) + end + + should 'retry on timeout' do + assert Stripe.should_retry?(RestClient::RequestTimeout.new, 0) + end + + should 'retry on a conflict' do + assert Stripe.should_retry?(RestClient::Conflict.new, 0) + end + + should 'not retry at maximum count' do + refute Stripe.should_retry?(RuntimeError.new, Stripe.max_network_retries) + end + + should 'not retry on a certificate validation error' do + refute Stripe.should_retry?(RestClient::SSLCertificateNotVerified.new('message'), 0) + end + end + + context ".sleep_time" do should "should grow exponentially" do Stripe.stubs(:rand).returns(1) Stripe.stubs(:max_network_retry_delay).returns(999) @@ -793,7 +799,6 @@ module Stripe assert_equal(base_value * 4, Stripe.sleep_time(3)) assert_equal(base_value * 8, Stripe.sleep_time(4)) end - end end end