Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
r-martins commentedMay 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 thepart of code where you would change the max_duration for the retries is never reached, unless the $options['max_duration'] isnot 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. |
r-martins commentedMay 18, 2022
To think... We can write another Strategy and deal with it in the With the current What do you think? |
r-martins left a comment
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 tothis comment. I didn't know I could review.
nicolas-grekas commentedMay 18, 2022
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 likeStripe's Status: needs review |
r-martins commentedMay 19, 2022
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