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] fix management of shorter-than-requested timeouts with AmpHttpClient#36960
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
| Loop::cancel($delay); | ||
| } | ||
| while (true) { | ||
| self::$delay =Loop::delay(1000 *$remaining, [Loop::class,'stop']); |
nicolas-grekasMay 25, 2020 • 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.
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.
@kelunik there is something I don't get here that you might want to check once this PR is merged. It looks like the delay defined here is not respected.
To see what I mean, checkout this PR, then adddump([$timeout, $remaining]); after the second "if" below, and run tests with./phpunit src/Symfony/Component/HttpClient/ --filter Amp.*NotATimeout.
You'll see something like:
array:2 [ 0 => 0.9 1 => 0.87336587905884]which means that the delay expired0.87336587905884 seconds earlier than expected.
Do I miss something or could this be a bug in AMP's event loop?
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.
I've added two failing tests inamphp/amp#319, one of those is expected to fail, the other isn't.
fabpot commentedMay 26, 2020
Thank you@nicolas-grekas. |
…Client (nicolas-grekas)This PR was merged into the 5.1 branch.Discussion----------[HttpClient] improve monitoring of timeouts with AmpHttpClient| Q | A| ------------- | ---| Branch? | 5.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replaces#36960Commits-------548818d [HttpClient] improve monitoring of timeouts with AmpHttpClient
This will fix the most transient test in our CI, which was not a false positive.