Retry on some non-2xx HTTP status codes

[rhodgkins]

Hi,

Just wanted to check why got doesn't do this...!

I thought the retries option would also be called HTTP errors but this doesn't appear to be the case (Promise / as stream) - is this by design?

I'd like got to retry certain 5xx error codes.

Cheers,

Rich

[petermetz]

#421

[sindresorhus]

I think we should do this.

I propose is that we retry on the following methods and status codes by default:

Methods: GET PUT HEAD DELETE OPTIONS TRACE
Status codes: 408 413 429 500 502 503 504
(Sourced from experience and other request clients)

We also need to respect the Retry-After header.

Related issue: #379

@lukechilds @reconbot @floatdrop @brandon93s Thoughts?

[reconbot]

Retry after support is awesome

[lukechilds]

One potential side effect to be aware of, especially if we're thinking of enabling this by default:

Consider you reach the request limit quota for the API you're using and they return 429 Too Many Requests with a Retry-After header set for when your quota resets next month.

This would mean, if we by default retry 429s and respect Retry-After, that making a request with Got to your API once you've hit your quota would return a Promise that may not resolve for up to 30 days in the future.

That doesn't seem very intuitive and could cause bugs that would be a nightmare to track down.

This seems like a great feature but might be better as opt in or not whitelisting 429.

[reconbot]

I think automatically retrying is probably not a great idea in general, this would be a good time to change that behavior.

…

On Sun, Feb 11, 2018, 10:42 AM Luke Childs ***@***.***> wrote: One potential side effect to be aware of, especially if we're thinking of enabling this by default: Consider you reach the request limit quota for the API you're using and they return 429 Too Many Requests with a Retry-After header set for when your quota resets next month. This would mean, if we by default retry 429s and respect Retry-After, that making a request with Got to your API once you've hit your quota would return a Promise that may not resolve for up to 30 days in the future. That doesn't seem very intuitive and could cause bugs that would be a nightmare to track down. This seems like a great feature but might be better as opt in or not whitelisting 429. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbl7-8eIcQ1WuaJLl-2zgTRhB62q1ks5tTwplgaJpZM4QgQwm> .

[sindresorhus]

Networks are inherently unreliable. I think retrying by default makes sense, but we should be very conservative with the default.

[brandon93s]

Methods: GET PUT HEAD DELETE OPTIONS TRACE

This list is sensible, as these methods should be safe or idempotent.

Would this functionality be rolled up into the existing retries option, or separated for more control? For API simplicity it makes sense to combine them, but the proposed functionality is more likely to need disabling than the current implementation.

Status codes: 408 413 429 500 502 503 504

413 may be an odd one to retry since the retry would be sent with an identical payload.

[sindresorhus]

Would this functionality be rolled up into the existing retries option, or separated for more control? For API simplicity it makes sense to combine them

I was thinking of combining them, but also allow passing an object where people can customize:

retries: {
	retry: Number | Function,
	methods: string[],
	statusCodes: number[]
}

Thoughts?

but the proposed functionality is more likely to need disabling than the current implementation.

Why do you think so?

413 may be an odd one to retry since the retry would be sent with an identical payload.

I agree. Let's take it out.

[sindresorhus]

Actually, regarding 413:

6.5.11. 413 Payload Too Large

The 413 (Payload Too Large) status code indicates that the server is
refusing to process a request because the request payload is larger
than the server is willing or able to process. The server MAY close
the connection to prevent the client from continuing the request.

** If the condition is temporary, the server SHOULD generate a
Retry-After header field to indicate that it is temporary and after
what time the client MAY try again.**

- https://tools.ietf.org/html/rfc7231#section-6.5.11

So it seems we could handle 413 as long as it has a Retry-After header and the time specified there is below the set timeout. Am I interpreting it correctly?

[brandon93s]

Agreed, retry 413 if and only if (iff) the response provides a Retry-After header. Retry-After could probably serve as a base case to enable retry, regardless of error, for the HTTP methods mentioned before.

I was thinking of combining them, but also allow passing an object where people can customize

Looks good to me. This can also be made backwards compatible by mapping retries Number | Function to retry.retries when we normalize arguments.

Why do you think so?

The aforementioned errors should be safe, but I've encountered some fairly non-standard APIs where I wouldn't want to retry on a 500 for example because I'm not confident the method is safe / idempotent. But, being able to specify methods and statusCodes solves for this.

[alixaxel]

@sindresorhus I'd like to use auto-retries on a 202 response (since it's async).

However, it seems that statusCodes only plays an effect if it's >= 400, is that right?

[robbieaverill]

Hi everyone, I came across this on a hunch that requests were automatically retried. @lukechilds said in his comment that:

That doesn't seem very intuitive and could cause bugs that would be a nightmare to track down.

I have to say I entirely agree. It depends on how you're using this library, but I'd much rather tell my users that the request was rate limited than allow it to infinitely keep retrying. I've disabled it in my config, but thought I'd add my two cents here.

Thanks for creating this library, it's great!

[szmarczak]

He was referring to retrying on 429 status code

[robbieaverill]

Sorry if I wasn’t clear, so was I