[HttpClient] Added RetryHttpClient

[jderusse]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	/
License	MIT
Doc PR	TODO

This PR adds a new HttpClient decorator to automatically retry failed requests.

When calling API, A very small % of requests are expected to timeout due to transient network issues. Some providers like AWS recommends retrying these requests and use a lower connection timeout so that the clients can fail fast and retry.

I used the almost the same configuration as Messenger

framework:
    http_client:
        default_options:
            retry_failed:
                enabled: true // default false
                decider_service: null
                backoff_service: null
                http_codes: [423, 425, 429, 500, 502, 503, 504, 507, 510]
                max_retries: 3
                delay: 1000
                multiplier: 2
                max_delay: 0
                response_header: true                                  
        scoped_clients:
            github:
                scope: 'https://api\.github\.com'
                retry_failed:
                    max_delay: 2000                                          

[javiereguiluz]

Nice feature!

A quick question: would end-users need to change their code to use this special HTTP client instead of the default one? If that's the case, I find it too cumbersome. What if we provide this feature via a config option of any HTTP client?

'retry_failed' => [
    'max_retries' => 2,             // -1 = infinite
    'wait_after_retries' => 10,     // seconds
    'retry_model' => 'exponential', // 'linear' or 'exponential' (== "exponential backoff") 
]

[jderusse]

Refactor implementation to use AsyncDecoratorTrait.
Added a RetryStrategy (very similar to the one in Messenger)

[jderusse]

This PR is RFR, thank you for the suggestion @javiereguiluz and thank you @nicolas-grekas for your help.

[stof]

In case of a 429 response, should we automatically try to read the Retry-After header ?

Also, should we have a single RetryStrategyInterface covering both the check whether retry should be done and the computation of the backoff delay ? Or should they be separate interfaces that can be replaced separately ? I see potential for customizing conditions for the retry while still keeping exponential backoff (for instance checking the request method to retry only idempotent requests)

[stof]

Btw, do we have access to the request when deciding to retry ? Or is is impossible to implement the case of attempting retry only for idempotent methods ?

[jderusse]

In case of a 429 response, should we automatically try to read the Retry-After header ?

Good idea, I added it. (I'm not sure how it should behave in case of conflict with delay computed by the backof...)

Also, should we have a single RetryStrategyInterface covering both the check whether retry should be done and the computation of the backoff delay ? Or should they be separate interfaces that can be replaced separately ? I see potential for customizing conditions for the retry while still keeping exponential backoff (for instance checking the request method to retry only idempotent requests)

Yeah, I hesitate a lot to split this interface in 2:

interface Decider
function shouldRetry(); // deal with exception and http status

interface Strategy
function isRetryable(); // retryCount < maxRetryCount
function getWaitingTime(); 

[stof]

In case of a 429 response, should we automatically try to read the Retry-After header ?

Good idea, I added it. (I'm not sure how it should behave in case of conflict with delay computed by the backof...)

https://github.com/hashicorp/go-retryablehttp makes the retry-after header win (this is where I got this idea btw)

Yeah, I hesitate a lot to split this interface in 2:

If we look in the go implementation (see just above in my message), the handling of a maximum number of retry is managed by the client itself rather than being delegated to an extension point (there isn't really a point changing the way a max_retries is checked, and you definitely don't want to allow removing that check). This leaves with the Decider interface with one method, and the BackOff interface with only getWaitingTime (or maybe getDelay would be a better name)

[jderusse]

I split the interface in 2:

1. DeciderInterface {function shouldRetry():bool;}

• HttpCodeDecider implements DeciderInterface which return true when the response code belongs to a defined list

2. BackOffInterface {function getDelay(): int;}

• ExponentialBackOff implements BackOffInterface (previously named Multiplier)
• HttpHeaderBackOff implements BackOffInterface that use the HttpHeader to define the delay (and use a BackOffInterface fallback when header is not set)

[nicolas-grekas]

Some more thoughts :)
Could deserve a line in the changelog of fwb.

[Nyholm]

Just a super minor

[fabpot]

Retry failed request support cannot be enabled as version 5.2+ of the HTTP Client component is required.

I would remove the composer command suggestion.

[fabpot]

Thank you @jderusse.