feat(request): optionally retry all network failures

[nbrustein]

If Stripe. max_retries_on_network_failure is set to a value greater than 0, then any request that fails on a connection error will be retried (with the exception of RestClient::SSLCertificateNotVerified).

If the retry succeeds, then the response will be returned just as if it succeeded on the first try. If the request still fails after Stripe. max_retries_on_network_failure attempts, then the last error will be raised.

Since it is not safe to retry posts with out an idempotency_key, this code also ensures that there is always an idempotency_key if max_retries_on_network_failure is greater than 0.

[kyleconroy]

@nbrustein Thanks for the PR. Right now, we're not ready to add official retry support to the bindings. We are, however, thinking about the best way to add this in the future. Stay tuned!

[nbrustein]

Could you tell me if there's anything special I would need to look out for if I was gonna start using something like this now? Any reason this approach would not work or be unsafe?

[booleanbetrayal]

Would love to see this or a similar mechanism in place soon!

[kyleconroy]

@nbrustein Nope, nothing inherently wrong with your approach. To prevent tight loops, you should put in exponential backoff when retrying requests.

[nbrustein]

@kyleconroy I just wanted to come back and hassle you one more time about pulling this in. We're trying to update our stripe gem today, and we have to deal with the fact that we have this special retry logic that we have to make sure isn't conflicting with any other changes.

I don't really understand your explanation for rejecting it. It sounds like you're saying "we think this is a good feature, but we'd rather write it ourselves instead of having someone else do it for us."

I don't mean to be annoying. I know that it's work for you guys to look more closely at this and decide whether to pull it in, but it's good functionality that would be useful to all of your users. It's done and it works. We've tested it in production for a couple months.

If I put in the exponential backoff would you merge it into master?

[kyleconroy]

There is still quite a bit of work to be done with this patch before we can merge in into master. A few things off the top of my head:

• rebase on master and ensure all tests pass (current patch fails)
• add exponential backoff instead of retrying in a tight loop
• only add idempotency tokens for POST requests (as DELETE and GET requests are already idempotent)
• document the retry behavior in the README

We'll also need to take a pass over the proposed interface and make sure it's something we're comfortable committing to.

[nbrustein]

@kyleconroy Thanks for your suggestions. I've resubmitted this as a new PR: #353