[HttpClient] provide response body to the RetryDecider #38289

jderusse · 2020-09-24T11:29:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	yes but for not-yet released 5.2 feature
Tickets	/
License	MIT
Doc PR	TODO

Some servers, like AWS, does not always return standard HTTP code. The strategy needs to parse the body to take a decision.
example:

400 
x-amzn-requestid: XXXXX
content-type: application/x-amz-json-1.1
content-length: 58
date: Thu, 24 Sep 2020 11:17:35 GMT
connection: close

{"__type":"ThrottlingException","message":"Rate exceeded"}

This PR update the RetryDeciderInterface interface to let the decider notifying the Client when it need the body to take a decision. in that case, the Client, fetch te client, and call again the decider with the full body.

usage

class Decider implements RetryDeciderInterface {
    public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent, \Throwable $throwable = null): ?bool
    {
        if (null !== $throwable) {
            return true;
        }
        if (in_array($responseCode, [423, 425, 429, 500, 502, 503, 504, 507, 510])) {
            return true;
        }
        if (
            $responseCode !== 400
            || $headers['content-type'][0] ?? null !== 'application/x-amz-json-1.1' 
            || (int) $headers['content-length'][0] ?? '0' > 1024
        ) {
            return false;
        }
        if (null === $responseContent) {
            return null; // null mean no decision taken and need to be called again with the body
        }

        $data = json_decode($responseContent, true);

        return $data['__type'] ?? '' === 'ThrottlingException';
    }
}

nicolas-grekas

content-type: application/x-amz-json-1.1

for my understanding, this means you're going to add a decider in async-aws that will chck both the status code and this specific content-type to decide to wait for the body, isn't it?

src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php

src/Symfony/Component/HttpClient/RetryableHttpClient.php

src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php

src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php

jderusse · 2020-10-01T14:10:45Z

for my understanding, this means you're going to add a decider in async-aws that will chck both the status code and this specific content-type to decide to wait for the body, isn't it?

yes. The decider should avoid as possible downloading the body. It can take a quick decision when statusCode is 2xx, 429, 5xx... Or content-type is unknow or content-length is too big.

but in some case (status = 400 + contentType = json + content-length < XXX) then it'll ask to the client to provide the body to take the decision.

src/Symfony/Component/HttpClient/RetryableHttpClient.php

nicolas-grekas

(I force-pushed a patch on AsyncResponse and confirmed it works)

jderusse · 2020-10-01T15:54:05Z

(I force-pushed a patch on AsyncResponse and confirmed it works)

Confirmed, it works. I pushed a commit about fixing wrong exception namespace

fabpot · 2020-10-02T05:36:29Z

Thank you @jderusse.

bohanyang · 2020-10-02T07:29:00Z

The body is read once so getContent don't work after that:

PHP Fatal error:  Uncaught Symfony\Component\HttpClient\Exception\TransportException: Cannot get the content of the response twice: buffering is disabled.

Sorry if my usage is wrong...

<?php

declare(strict_types=1);

use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\HttpClient\Retry\ExponentialBackOff;
use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider;
use Symfony\Component\HttpClient\RetryableHttpClient;

require __DIR__ . '/vendor/autoload.php';

$client = new RetryableHttpClient(new CurlHttpClient(), new HttpStatusCodeDecider(), new ExponentialBackOff(), 1);
$resp = $client->request('GET', 'https://httpbin.org/status/429');
var_dump($resp->getContent(false));

…derusse) This PR was merged into the 5.2-dev branch. Discussion ---------- [HttpClient] Fix exception triggered with AsyncResponse | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #38289 (comment) | License | MIT | Doc PR | / When a request is replayed with the AsyncResponse and the 2nd request failed, users get an exception even when they pass `$throw=false`. In fact, there are 2 exceptions: 1. Cannot get the content of the response twice: buffering is disabled. https://github.com/symfony/symfony/blob/73ca97c97ab4c4fc9a7bca5fb4339fca22e0cfa1/src/Symfony/Component/HttpClient/Response/CommonResponseTrait.php#L51-L68 Because CommonResponseTrait::$content is null and `self::stream` yield only the LastChunk. The content is stays null 2. HTTP/2 429 returned for "https://httpbin.org/status/429" https://github.com/symfony/symfony/blob/73ca97c97ab4c4fc9a7bca5fb4339fca22e0cfa1/src/Symfony/Component/HttpClient/Response/AsyncResponse.php#L209-L225 Because on the second request passthru is null, it didn't disable the the exception thrown on destruct for the wrapped response. This Commits ------- 3aedb51 [HttpClient] Fix exception triggered with AsyncResponse

…derusse) This PR was merged into the 5.2-dev branch. Discussion ---------- [HttpClient] Fix exception triggered with AsyncResponse | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | symfony/symfony#38289 (comment) | License | MIT | Doc PR | / When a request is replayed with the AsyncResponse and the 2nd request failed, users get an exception even when they pass `$throw=false`. In fact, there are 2 exceptions: 1. Cannot get the content of the response twice: buffering is disabled. https://github.com/symfony/symfony/blob/73ca97c97ab4c4fc9a7bca5fb4339fca22e0cfa1/src/Symfony/Component/HttpClient/Response/CommonResponseTrait.php#L51-L68 Because CommonResponseTrait::$content is null and `self::stream` yield only the LastChunk. The content is stays null 2. HTTP/2 429 returned for "https://httpbin.org/status/429" https://github.com/symfony/symfony/blob/73ca97c97ab4c4fc9a7bca5fb4339fca22e0cfa1/src/Symfony/Component/HttpClient/Response/AsyncResponse.php#L209-L225 Because on the second request passthru is null, it didn't disable the the exception thrown on destruct for the wrapped response. This Commits ------- 3aedb51dd8 [HttpClient] Fix exception triggered with AsyncResponse

carsonbot added Status: Needs Review Feature Deprecation labels Sep 24, 2020

jderusse changed the title ~~[HtppClient] provide body to retry decider~~ [HtppClient] provide response body to the RetryDecider Sep 24, 2020

jderusse force-pushed the retry-body branch 3 times, most recently from 1c29567 to cde9938 Compare September 24, 2020 11:56

nicolas-grekas added this to the next milestone Sep 24, 2020

nicolas-grekas changed the title ~~[HtppClient] provide response body to the RetryDecider~~ [HttpClient] provide response body to the RetryDecider Sep 24, 2020

nicolas-grekas reviewed Sep 29, 2020

View reviewed changes

src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Sep 30, 2020

View reviewed changes

src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php Outdated Show resolved Hide resolved

jderusse force-pushed the retry-body branch from ebd5c2b to c85bbb0 Compare October 1, 2020 14:06

nicolas-grekas reviewed Oct 1, 2020

View reviewed changes

src/Symfony/Component/HttpClient/RetryableHttpClient.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Oct 1, 2020

View reviewed changes

src/Symfony/Component/HttpClient/RetryableHttpClient.php Outdated Show resolved Hide resolved

nicolas-grekas force-pushed the retry-body branch 2 times, most recently from d4da9b2 to d0844c2 Compare October 1, 2020 15:31

nicolas-grekas approved these changes Oct 1, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 1, 2020

[HttpClient] provide response body to the RetryDecider

321be58

jderusse force-pushed the retry-body branch from 5f729d6 to 321be58 Compare October 1, 2020 15:55

fabpot merged commit b35bbdb into symfony:master Oct 2, 2020

fabpot mentioned this pull request Oct 2, 2020

[HttpClient] provide response body to the RetryDecider symfony/symfony-docs#14317

Closed

nicolas-grekas mentioned this pull request Oct 2, 2020

[HttpClient] AsyncResponse: Underlying response still checks status code on destruct even after getContent(false) #37663

Closed

jderusse mentioned this pull request Oct 2, 2020

[HttpClient] Fix exception triggered with AsyncResponse #38378

Merged

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

jderusse deleted the retry-body branch October 15, 2020 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[HttpClient] provide response body to the RetryDecider #38289

[HttpClient] provide response body to the RetryDecider #38289

Uh oh!

jderusse commented Sep 24, 2020

Uh oh!

nicolas-grekas left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jderusse commented Oct 1, 2020

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas left a comment

Uh oh!

jderusse commented Oct 1, 2020

Uh oh!

fabpot commented Oct 2, 2020

Uh oh!

bohanyang commented Oct 2, 2020

Uh oh!

Uh oh!

Uh oh!

[HttpClient] provide response body to the RetryDecider #38289

[HttpClient] provide response body to the RetryDecider #38289

Uh oh!

Conversation

jderusse commented Sep 24, 2020

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jderusse commented Oct 1, 2020

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

jderusse commented Oct 1, 2020

Uh oh!

fabpot commented Oct 2, 2020

Uh oh!

bohanyang commented Oct 2, 2020

Uh oh!

Uh oh!