[HttpClient] Retry safe requests using HTTP/1.1 when HTTP/2 fails #34199
Conversation
6a04fd0 to
8fbe1cd
Compare
|
Why making 3 retries when HTTP/2 fails ? |
8fbe1cd to
4726d0f
Compare
4726d0f to
9f7cd66
Compare
updated to 2: the first on HTTP/2, the last at HTTP/1.1 |
| @@ -66,6 +68,7 @@ public function __construct(array $defaultOptions = [], int $maxHostConnections | |||
| } | |||
|
|
|||
| $this->multi = $multi = new CurlClientState(); | |||
| self::$curlVersion = self::$curlVersion ?? curl_version(); | |||
There was a problem hiding this comment.
Calling curl_version() repeatedly is leaking memory - I suppose the array created by the extension is not collected as userland arrays.
There was a problem hiding this comment.
I suggest to also report that as a PHP bug.
There was a problem hiding this comment.
I don't think that's a bug - token_get_all() has the same "leak", reclaimed when calling https://www.php.net/manual/en/function.gc-mem-caches.php
| $this->errorMessage = null !== $error ? $error->getMessage() : 'Reading from the response stream reached the idle timeout.'; | ||
|
|
||
| if (\is_string($error)) { | ||
| $this->errorMessage = $error; |
There was a problem hiding this comment.
The message now contains the corresponding URL.
|
|
||
| curl_setopt($ch, CURLOPT_HEADERFUNCTION, null); | ||
| curl_setopt($ch, CURLOPT_READFUNCTION, null); | ||
| curl_setopt($ch, CURLOPT_INFILE, null); |
There was a problem hiding this comment.
This is not needed - I've actually seen complains from curl_multi_exec() about this state change while processing the responses.
| @@ -150,8 +148,6 @@ public function __construct(CurlClientState $multi, $ch, array $options = null, | |||
| public function getInfo(string $type = null) | |||
| { | |||
| if (!$info = $this->finalInfo) { | |||
| self::perform($this->multi); | |||
There was a problem hiding this comment.
Performing here totally kills performances when processing a large number of requests.
| public function getContent(bool $throw = true): string | ||
| { | ||
| $performing = self::$performing; | ||
| self::$performing = $performing || '_0' === curl_getinfo($this->handle, CURLINFO_PRIVATE); |
There was a problem hiding this comment.
Performing here while the content is already available also kills performances when processing a large number of requests.
| $id = (int) $ch = $info['handle']; | ||
| $waitFor = @curl_getinfo($ch, CURLINFO_PRIVATE) ?: '_0'; | ||
|
|
||
| if (\in_array($result, [\CURLE_SEND_ERROR, \CURLE_RECV_ERROR, /*CURLE_HTTP2*/ 16, /*CURLE_HTTP2_STREAM*/ 92], true) && $waitFor[1] && 'C' !== $waitFor[0]) { |
There was a problem hiding this comment.
The consts aren't declared in the PHP extension - yet these HTTP2 error numbers can still be yielded.
| @@ -80,8 +80,6 @@ public function __construct(NativeClientState $multi, $context, string $url, $op | |||
| public function getInfo(string $type = null) | |||
| { | |||
| if (!$info = $this->finalInfo) { | |||
| self::perform($this->multi); | |||
There was a problem hiding this comment.
for consistency with CurlResponse
…2 fails (nicolas-grekas) This PR was merged into the 4.3 branch. Discussion ---------- [HttpClient] Retry safe requests using HTTP/1.1 when HTTP/2 fails | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - cURL support of HTTP/2 is not as robust as HTTP/1.1. When doing >1k requests, the stream can break for buggy reasons. New versions of cURL are fixed already, but let's make our logic more resilient anyway, and switch to HTTP/1.1 when a *safe* request fails for send/recv reasons. Commits ------- 9f7cd66 [HttpClient] Retry safe requests when then fail before the body arrives
|
@nicolas-grekas but isn't it still only 1 retry, not 2 ? The first request is not a retry. And if your expectation is to count the initial request as well, then the issue is using |
H2 (initial value) => retry with HTTP/2 again and decrement to H1 so, that's two retries? or do you mean something else? |
cURL support of HTTP/2 is not as robust as HTTP/1.1. When doing >1k requests, the stream can break for buggy reasons. New versions of cURL are fixed already, but let's make our logic more resilient anyway, and switch to HTTP/1.1 when a safe request fails for send/recv reasons.