Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python retry mechanism #470

Merged
merged 12 commits into from
Sep 7, 2018
Merged

Conversation

mickjermsurawong-stripe
Copy link
Contributor

Summary:

  • Implement retry mechanism at abstract HTTPClient class, and use delegated request call to each client implementation.
  • Only consider errors from requests http-client, but not others due to overwhelming majority of our clients. This design allows future extension the support to other http-clients. (As opposed to urllib3 Retry that is coupled at the execution invocation.
  • Retry conditions follow other language implementations (Ruby[0]/Go[1])
    • Retry on 409 resource conflict (both)
    • Retry on timeout/connection (only Ruby)
  • Exponential backoff delay follows other language implementations (Ruby[2]/Go[3])
  • Added idempotency key only for POST method, and when it is not given in the header

Concerns:

  • Not sure where's best to put delay constants in config. Currently made as static class var
  • I'm not sure how to access the actual requests module from within test, and that results in mocking kinds of exception classes that we want to handle (existing tests also rely on the similar mechanism)

Motivation:

[0] Ruby retry conditions
https://github.com/stripe/stripe-ruby/blob/ec66c3f0f44274f885de8d13de5dce2657932121/lib/stripe/stripe_client.rb#L61
[1] Go retry conditions
https://github.com/stripe/stripe-go/blob/442690c3856640e4913163f506f0070561f3368d/stripe.go#L495
[2] Ruby sleep
https://github.com/stripe/stripe-ruby/blob/ec66c3f0f44274f885de8d13de5dce2657932121/lib/stripe/stripe_client.rb#L80
[3] Go sleep
https://github.com/stripe/stripe-go/blob/442690c3856640e4913163f506f0070561f3368d/stripe.go#L511

@mickjermsurawong-stripe
Copy link
Contributor Author

cc @jonathan-s

@mickjermsurawong-stripe
Copy link
Contributor Author

cc @stripe/api-libraries

@mickjermsurawong-stripe
Copy link
Contributor Author

mickjermsurawong-stripe commented Sep 7, 2018

requests errors to retry

# ConnectionError
requests.exceptions.ConnectionError
requests.exceptions.ProxyError
requests.exceptions.ConnectTimeout

# TimeoutError
requests.exceptions.Timeout
requests.exceptions.ReadTimeout
requests.exceptions.ConnectTimeout

requests errors not to retry

# Connection Error
requests.exceptions.SSLError

# ValueError
requests.exceptions.InvalidHeader
requests.exceptions.InvalidSchema 
requests.exceptions.InvalidURL
requests.exceptions.MissingSchema
requests.exceptions.URLRequired

# TypeError
requests.exceptions.StreamConsumedError

# Only when setting response.raise_for_status()
requests.exceptions.HTTPError

# Encode/Decode
requests.exceptions.ChunkedEncodingError
requests.exceptions.ContentDecodingError 

# Redirect-related
requests.exceptions.TooManyRedirects
requests.exceptions.UnrewindableBodyError

# Retry
requests.exceptions.RetryError

http://docs.python-requests.org/en/master/_modules/requests/exceptions/

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @mickjermsurawong-stripe! Thanks for working on this.

stripe/http_client.py Outdated Show resolved Hide resolved
stripe/http_client.py Outdated Show resolved Hide resolved
stripe/http_client.py Show resolved Hide resolved
@ob-stripe
Copy link
Contributor

Alright, this looks great to me, but the feature is complex enough that I'd like to get another set of eyes on it.

r? @brandur-stripe

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple minor comments, but thanks for being so thorough on this one! Looks excellent.

ptal @mickjermsurawong-stripe

stripe/http_client.py Show resolved Hide resolved
stripe/http_client.py Outdated Show resolved Hide resolved
tests/test_http_client.py Show resolved Hide resolved
@mickjermsurawong-stripe
Copy link
Contributor Author

Added comment/logging
ptal @brandur-stripe

@brandur-stripe
Copy link
Contributor

LGTM! Thanks.

@mickjermsurawong-stripe
Copy link
Contributor Author

self-approve after lgtm ^

@mickjermsurawong-stripe
Copy link
Contributor Author

Released in 2.8 fb8117d

@kyleries
Copy link

kyleries commented Apr 1, 2020

Ended up needing this feature and came across this thread to check out the implementation. Wanted to drop a belated "thank you" for this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants