Retry logic broken in RetryDeciderTrait for $httpRetryMessages

[paslandau]

Environment details

• OS: any
• PHP version: any
• Package name and version: core

Description

The default retry logic of RetryDeciderTrait is broken. The code assumes that the exception message contains a valid json string that can be examined for a reason key.

This assumption is incorrect, because the underlying Guzzle client will provide a "pretty" exception message that is also truncated (see guzzle/guzzle#2185 ). The actual content of the response can be retrieved via $ex->getResponse()->getBody()->getContents();.

This issue mainly effects the RequestWrapper as it the RetryDeciderTrait is used here to retrieve the default retry function.

Steps to reproduce

The following unit test reproduces the problem. Please note that I deliberatly use the Guzzle Mock Handler in order to get an actual RequestException from Guzzle, triggered by a 4xx/5xx status code:

    /**
     * @dataProvider shouldRetryDefaultRetryableErrorsDataProvider
     */
    public function testShouldRetryDefaultRetryableErrors(array $responses, \Exception $e = null)
    {
        $mock = new MockHandler();
        foreach ($responses as $response) {
            $mock->append($response);
        }

        // add valid respone
        $mock->append(new Response(200));

        $handler     = HandlerStack::create($mock);
        $client      = new Client(['handler' => $handler]);
        $httpHandler = HttpHandlerFactory::build($client);

        $requestWrapper = new RequestWrapper([
            'credentialsFetcher' => new AnonymousCredentials(),
            'httpHandler'        => $httpHandler,
            'restDelayFunction'  => function ($delay) {
            },
        ]);

        if ($e !== null) {
            $this->expectException(get_class($e));
            $this->expectExceptionMessage($e->getMessage());
        }
        $requestWrapper->send(new Request('GET', 'http://www.example.com'));
        $this->assertTrue(true);
    }

    public function shouldRetryDefaultRetryableErrorsDataProvider()
    {
        // Note:
        // Examples taken from RetryDeciderTrait
        return [
            "reason: 500"                   => [
                "responses" => [
                    new Response(500),
                ],
                "exception" => null,
            ],
            "reason: 502"                   => [
                "responses" => [
                    new Response(502),
                ],
                "exception" => null,
            ],
            "reason: 503"                   => [
                "responses" => [
                    new Response(503),
                ],
                "exception" => null,
            ],
            "reason: rateLimitExceeded"     => [
                "responses" => [
                    new Response(429, [], json_encode([
                        "error" => [
                            "errors" => [
                                [
                                    "reason" => "rateLimitExceeded",
                                ],
                            ],
                        ],
                    ])),
                ],
                "exception" => null,
            ],
            "reason: userRateLimitExceeded" => [
                "responses" => [
                    new Response(429, [], json_encode([
                        "error" => [
                            "errors" => [
                                [
                                    "reason" => "userRateLimitExceeded",
                                ],
                            ],
                        ],
                    ])),
                ],
                "exception" => null,
            ],
            "non-retryable error"           => [
                "responses" => [
                    new Response(400, [], json_encode([
                        "error" => [
                            "errors" => [
                                [
                                    "reason" => "badRequest",
                                ],
                            ],
                        ],
                    ])),
                ],
                "exception" => new BadRequestException("badRequest"),
            ],
        ];
    }

Proposed solution

The RetryDeciderTrait should check if the Exception is an instance of a Guzzle RequestException and use the reponse body in that case to check for the error:

            $message = $ex->getMessage();
            if ($ex instanceof RequestException) {
                $message = $ex->getResponse()->getBody()->getContents();
            }
            $message = json_decode($message, true);

instead of

            $message = json_decode($ex->getMessage(), true);

I would provide a correponsing pull request if this gets accepted.

PS: I guess #1273 is actually already resolved?

[dwsupplee]

@paslandau your proposed solution is exactly what we need.

A PR would be wonderful, but I am also happy to hop on getting this resolved as I'd like to get this fixed ASAP. Please let me know.

[paslandau]

@dwsupplee

Unfortunately currently no access to a laptop - feel free do fix it yourself. I'll get that contribution another time ;)