-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Honor "max_duration" when replacing requests with async decorators #46382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for that, @nicolas-grekas. $url = 'http://localhost:8888/sleep3.php';
$response = $client->request('POST', $url, [
'body' => ['foo'=>'bar'],
'max_duration' => 11
]); Then, the first and the second request fails after 6 seconds and the third would succeed after 5 seconds (total 6+6+5=17). But it turns out that the part of code where you would change the max_duration for the retries is never reached, unless the $options['max_duration'] is not set. The above request succeeded after around 20 seconds, not triggering the expected timeout error. If I remove If you want to use it, my stupid test script was: <?php
$count = (int)@file_get_contents('.count') == 0 ? 1 : (int)@file_get_contents('.count');
if ($count <=2) {
file_put_contents('.count', ++$count);
sleep(6);
echo '500';
http_response_code(500);
exit;
}
if ($count > 2) {
sleep(5);
unlink('.count');
echo $count . ' attempt. SUCCESS!';
exit;
} Serve it with php -S and do the test above. |
To think... We can write another Strategy and deal with it in the With the current What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to this comment. I didn't know I could review.
4617f9f
to
c81d116
Compare
Thanks for checking @r-martins! This is now fixed.
This reasoning applies to any duration, 0.2 or longer. I don't like the idea of adding a min-duration option because hey, what would be a sensible value for that?!
Retries do not happen for non idempotent requests by default. If your strategy retries eg POST requests, then this issue is unrelated to the max_duration as it can happen with any retries. To guard against these, you need to use something like Stripe's Status: needs review |
Thanks for the update @nicolas-grekas. I could test and it's working as expected now. Lastly, could you suggest a way to merge your PR in my project? I'm afraid waiting for a new release may take too long. I saw you have it separately, and also http-client is a separated project... Can you advise? |
Instead of #46330