Retry throttled calls on a provided HTTP client

[nicolas-grekas]

AbstractApi wraps its HTTP client in a RetryableHttpClient with an AwsRetryStrategy, but only when it builds that client itself:

if (!isset($httpClient)) {
    $httpClient = HttpClient::create();
    if (class_exists(RetryableHttpClient::class)) {
        $httpClient = new RetryableHttpClient($httpClient, new AwsRetryStrategy(...), 3, $this->logger);
    }
}

So a caller that passes a client to set a timeout, a proxy, certificates, or to get the requests into a profiler, silently loses the AWS retry policy. That is not obvious from the outside: the constructor argument reads like "use this transport", not "and give up retrying".

It matters because the two policies are not interchangeable. Symfony's GenericRetryStrategy maps 0, 500, 504, 507 and 510 to IDEMPOTENT_METHODS, which does not include POST, and every AWS API call is a POST. It also never looks at a 400 body, where AWS reports throttling. Measured on the same throttled request, all POST:

response	AwsRetryStrategy	GenericRetryStrategy
500 Internal Server Error	4 attempts	1
504 Gateway Timeout	4	1
400 ThrottlingException	4	1
400 ProvisionedThroughputExceededException	4	1
400 ValidationException	1	1

This moves the wrapper out of the isset() branch so it applies to a given client too, keeping the error factory and the logger of the client it belongs to.

Symfony's SES, SQS, SNS, DynamoDb and KMS bridges all pass their application's client, so they are all in this case today. The alternative is for each of them to rebuild the wrapper, which duplicates the policy in five places and cannot reach the per-client error factory.

Worth noting it is a behaviour change: a caller who passes a client now gets up to three extra attempts on throttling and 5xx. A caller whose client already retries gets both layers.

[nicolas-grekas]

Inlined, thanks. CI was mine too: ### Changed landed on line 5, which BranchAliasTest reads as a patch-level change and so expected 1.30-dev. Moving it after ### Added fixes that and the ordering assertion together.

On muting the given client, I tried it in the Symfony PR and pulled it back out. It does work, including through a decorator, so a retryable client under a TraceableHttpClient is still reached:

Traceable(Retryable(Scoping(mock)))   muted    attempts=4
Retryable(Scoping(mock))              muted    attempts=4
Traceable(Native)                     caught InvalidArgumentException

and the multiplication it avoids is real, 16 attempts for one throttled request rather than 4.

What stopped me is that withOptions() returns a clone, so we would talk through a copy of the client the caller gave us rather than the instance itself:

plain mock muted      : original getRequestsCount()=0
via retryable, muted  : original mock getRequestsCount()=0
wrap only, no mute    : original mock getRequestsCount()=1

The second line is the awkward one: the clone happens even when a retryable client consumes the option, because RetryableHttpClient::withOptions() forwards to $this->client->withOptions([]). Traces still reach a profiler, since TraceableHttpClient shares its ArrayObject across clones, but anyone counting requests on the client they passed stops seeing them. That broke a test in Symfony that does exactly that.

So it is a real trade rather than a clear win, and it is your call which side of it async-aws wants. Happy to add it if you prefer the lower attempt count; the catch is safe to write now that HttpClientInterface::withOptions() is documented as throwing on an unsupported option (symfony/symfony#66183).

[nicolas-grekas]

Applied, you were right and my objection was not.

I had argued the mute detaches the caller from the client it passed, because withOptions() returns a clone. That is true only for state held in a scalar. Every real client shares the object its state lives in:

CurlHttpClient   $multi          : SHARED (CurlClientState)      <- same connection pool
NativeHttpClient $multi          : SHARED (NativeClientState)    <- same DNS cache
Traceable        $tracedRequests : SHARED (ArrayObject)          <- profiler intact
MockHttpClient   $requestsCount  : int, copied on clone

I had generalised from the last line, which is a test double.

One existing test did need a fix: testDiscoveredEndpoint mocks HttpClientInterface without stubbing withOptions(), so the bare mock returned a different instance and its expectation never fired. willReturnSelf() is the right way to mock a method returning static, so the test was under-specified rather than the change being wrong. Nothing else in the repo is affected: getRequestsCount() has no uses here, and the 370 files building a MockHttpClient do not assert on the instance they pass.

testProvidedHttpClientThatAlreadyRetriesDoesNotRetryTwice pins it, through a TraceableHttpClient so the option has to travel the chain rather than hit a RetryableHttpClient directly. Without the mute it fails with Failed asserting that 16 is identical to 4.

[nicolas-grekas]

The lowest job caught a real one: HttpClientInterface::withOptions() only exists from symfony/http-client-contracts 2.4, and src/Core/composer.json allows ^1.1.8 || ^2.0 || ^3.0. Calling it unconditionally would have been a fatal for anyone on an older contract, so it is now behind method_exists($httpClient, 'withOptions'), checked on the instance rather than the interface so an implementation that has it is still muted.

The two test failures had the same root: stubbing withOptions() on a mocked interface that does not declare it raises MethodCannotBeConfiguredException, and the no-double-retry assertion cannot hold when the mute cannot run. The stub is now conditional and that test skips below 2.4.

[stof]

Which version of symfony/http-client corresponds to contracts 2.4 ? Maybe we can bump the min version of the dependency if this is only for Symfony 4.4 which is long EOL

[stof]

Answering my question: it was added in symfony/http-client 5.3

[stof]

For reference, this was added in symfony/http-client 5.2, so this condition could be removed if we drop support for 4.4

[nicolas-grekas]

Bumped symfony/http-client to ^5.3 and symfony/http-client-contracts to ^2.4 in src/Core/composer.json, which lets the class_exists(RetryableHttpClient::class) guard go. Confirmed against the tags rather than from memory:

	added in
RetryableHttpClient	http-client 5.2 (absent at 5.1.0, present at 5.2.0)
HttpClientTrait::withOptions()	http-client 5.3
HttpClientInterface::withOptions() as a real method	contracts 3.0

That last row is why I kept the method_exists() check. Contracts 2.4 only added it as a @method annotation on the interface:

 * @method static withOptions(array $options) Returns a new instance of the client with new default options

so an implementation compiled against 2.x is not obliged to have it. Symfony's own clients all do from 5.3, and the check costs nothing, so it seemed better than requiring contracts ^3.0 and dropping Symfony 5 altogether. Happy to go that way instead if you would rather have the branch gone.

http-client 5.3 already requires contracts ^2.4, so the two constraints move together.