[HttpClient] Retry on timeout #38501
Conversation
jderusse
requested review from
dunglas,
lyrixx,
sroze and
xabbuh
as code owners
jderusse
force-pushed
the
retry-timeout
branch
from
6cf6980 to
fa0dadc
Compare
jderusse
force-pushed
the
retry-timeout
branch
from
fa0dadc to
fdcf9ee
Compare
jderusse
force-pushed
the
retry-timeout
branch
from
fdcf9ee to
0bed3b1
Compare
| */ | ||
| public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null) | ||
| public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, float $retryTimeout = null, LoggerInterface $logger = null) |
There was a problem hiding this comment.
what's about array $options ?
|
|
||
| return new AsyncResponse($this->client, $method, $url, $options, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, &$retryCount, &$content, &$firstChunk) { | ||
| return new AsyncResponse($this->client, $method, $url, $options + ['timeout' => $this->retryTimeout], function (ChunkInterface $chunk, AsyncContext $context) use (&$lastEventTime, $method, $url, $options, &$retryCount, &$content, &$firstChunk) { |
There was a problem hiding this comment.
If the user call the request method with an timeout option lower that retryTimeout, should'nt we use that timeout for the sub-request?
There was a problem hiding this comment.
I see what you want, we need a better solution. If you look at CommonResponseTrait::initialize(), you'll see that timeouts thrown during initialization are turned into errors. This is something that passthru filters don't have access to. This is what we should enhance instead. The initializer closure in AsyncResponse could ensure that an ErroChunk is passed to the passthru filter, so that the filter could act on it (dunno how exactly right now).
There was a problem hiding this comment.
Maybe that's all we need in this PR actually: be able to detect when a timeout is an error, and retry these. That would allow removing the retryTimeout argument.
This PR was merged into the 5.x branch. Discussion ---------- [HttpClient] fix decorating timeout errors | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Replaces #38501 | License | MIT | Doc PR | - /cc @jderusse Commits ------- 84cda3f [HttpClient] fix decorating timeout errors
This PR adds a new options to the RetryableHttpClient to retry IDLE sub-requests.