Retry based on Stripe-Should-Retry and Retry-After headers
#692
Conversation
rattrayalex-stripe
force-pushed
the
ralex/smarter-retries
branch
from
b63cba4 to
0d3037a
Compare
rattrayalex-stripe
force-pushed
the
ralex/smarter-retries
branch
from
d11d3dc to
82f107b
Compare
rattrayalex-stripe
dismissed
paulasjes-stripe’s stale review
optional chaining
There was a problem hiding this comment.
LGTM!
cc @brandur-stripe to check the logic and make sure it's the one we want to apply to all client libraries.
| @@ -228,21 +228,31 @@ StripeResource.prototype = { | |||
| return true; | |||
| } | |||
|
|
|||
| // Retry on conflict and availability errors. | |||
| if (res.statusCode === 409 || res.statusCode === 503) { | |||
| if (res.headers && res.headers['stripe-should-retry'] === 'false') { | |||
There was a problem hiding this comment.
Can we put some docs on this to describe generally what Stripe-Should-Retry is?
There was a problem hiding this comment.
Good idea, will do
| if (res.headers && res.headers['stripe-should-retry'] === 'false') { | ||
| return false; | ||
| } | ||
| if (res.headers && res.headers['stripe-should-retry'] === 'true') { |
There was a problem hiding this comment.
How does the interaction between Stripe-Should-Retry and Retry-After work?
I guess based on this condition being placed so early, if we're going to send Retry-After, then Stripe-Should-Retry is set to false or not included? If so, doesn't that mean we have to fall back on other heuristics to decide whether to retry (like status code)?
I wonder if we should have it so that if there's a Retry-After, Stripe-Should-Retry is still set to an appropriate value that indicates whether a retry is appropriate. If we went down that route, we'd have to include a && !res.headers['stripe-retry-after'] in this check.
There was a problem hiding this comment.
if we're going to send Retry-After, then Stripe-Should-Retry is set to false or not included?
I assume you meant "true" not "false, and yes: I can share the internal doc, but our API should hold the invariant that if Retry-After is set, Stripe-Should-Retry should not be set to false.
Were that to happen, though, I think Stripe-Should-Retry: false should take precedence and block a Retry-After: X, given that the likely intent of the latter is to ask clients to wait a little longer before retrying, and the intent of the former is to ask them not to retry at all (eg; because it will be futile or damaging).
In general, they're pretty orthogonal; technically, these should be separate PR's (I'm just lumping them together for convenience, since they came to mind at the same time).
Note that I think we'll ~always want to fall back to status code checks for at least some cases (eg; 500, 503, 504) in case the parts of our api stack that set Stripe-Should-Retry are not hit.
I could see a case for dropping the code for 409 handling here and moving that into the API...
However, in general, we should expect the client to retry in some cases that the server does not make a recommendation, for example, 429's, where we do not retry but the client may choose to (but would hopefully respect Retry-After).
lib/StripeResource.js
Outdated
| @@ -261,6 +271,11 @@ StripeResource.prototype = { | |||
| // But never sleep less than the base sleep seconds. | |||
| sleepSeconds = Math.max(initialNetworkRetryDelay, sleepSeconds); | |||
|
|
|||
| // And never sleep less than the time the API asks us to wait, assuming it's a reasonable ask. | |||
| if (Number.isInteger(retryAfter) && retryAfter <= 60) { | |||
There was a problem hiding this comment.
Can we constant-ize 60 here to make it more literate? Maybe MAX_RETRY_AFTER_WAIT?
rattrayalex-stripe
force-pushed
the
ralex/smarter-retries
branch
from
5c9b425 to
b24337d
Compare
|
Self-approving after adopting recommended changes |
As seen in stripe-node, adds support for the `Stripe-Should-Retry` header which is sent by the API when it explicitly would like us to either retry or _not_ retry. I'll add `Retry-After` separately at some point, but I punted it on it for now given that we're not using it yet. See: stripe/stripe-node#692
As seen in stripe-node, adds support for the `Stripe-Should-Retry` header which is sent by the API when it explicitly would like us to either retry or _not_ retry. I'll add `Retry-After` separately at some point, but I punted it on it for now given that we're not using it yet. See: stripe/stripe-node#692
As seen in stripe-node, adds support for the `Stripe-Should-Retry` header which is sent by the API when it explicitly would like us to either retry or _not_ retry. I'll add `Retry-After` separately at some point, but I punted it on it for now given that we're not using it yet. See: stripe/stripe-node#692
As seen in stripe-node, adds support for the `Stripe-Should-Retry` header which is sent by the API when it explicitly would like us to either retry or _not_ retry. I'll add `Retry-After` separately at some point, but I punted it on it for now given that we're not using it yet. See: stripe/stripe-node#692
r? @ob-stripe
cc @stripe/api-libraries @paulasjes-stripe @jlomas-stripe
This adds support for the
Stripe-Should-RetryandRetry-Afterheaders.This allows the API to specify specific cases when an automatic retry is recommended (such as
lock_timeouterrors) or not recommended (such as when our idempotency framework would replay the request anyway).It also allows the API to encourage slower retries.