mirror of
https://github.com/stripe/stripe-ruby.git
synced 2025-06-04 00:01:12 -04:00
Rework HTTP retry path + retry 409s
Two changes: 1. The HTTP retry path has been refactored to make retries on errors that are not RestClient exceptions possible by bringing the logic up a level. This also has the effect of somewhat simplifying the exception handling logic which can be somewhat difficult to reason about right now. 2. Retry on `RestClient::Conflict` (a 409 from the API) as discussed in issue #431. Fixes #431.
This commit is contained in:
Brandur
parent
be8466b47b
commit
3984246514
@ -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
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user