[HttpClient] Fix setting CURLMOPT_MAXCONNECTS
#58278
Conversation
| frankenphp: | ||
| image: dunglas/frankenphp:1.1.0 | ||
| ports: | ||
| - 80:80 | ||
| - 8681:81 | ||
| - 8682:82 | ||
| - 8683:83 | ||
| - 8684:84 | ||
| volumes: | ||
| - ${{ github.workspace }}:/symfony | ||
| env: | ||
| SERVER_NAME: 'http://localhost http://localhost:81 http://localhost:82 http://localhost:83 http://localhost:84' | ||
| CADDY_SERVER_EXTRA_DIRECTIVES: | | ||
| route /http-client* { | ||
| root * /symfony/src/Symfony/Component/HttpClient/Tests/Fixtures/response-functional/ | ||
| php_server | ||
| } |
There was a problem hiding this comment.
Backported from later versions of Symfony. This is needed since the PHP built-in server doesn't support persistent connections.
| $maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, 0 < $maxHostConnections ? $maxHostConnections : \PHP_INT_MAX) ? 0 : $maxHostConnections; | ||
| $maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, 0 < $maxHostConnections ? $maxHostConnections : \PHP_INT_MAX) ? 4294967295 : $maxHostConnections; |
There was a problem hiding this comment.
As @nicolas-grekas mentioned, zero doesn't mean unlimited. Instead, it sets the default behavior to four times the number of added handlers. This can be seen in the code:
unsigned int maxconnects = !data->multi->maxconnects ?
data->multi->num_easy * 4: data->multi->maxconnects;Also, since the variable is an int, PHP_INT_MAX cannot be used, as it’s too large on 64-bit systems.
There was a problem hiding this comment.
Really? curl has fixed size for long? Do we know if the number is signed? Aka 2147483647 would then be the max?
There was a problem hiding this comment.
@nicolas-grekas The variable is declared as unsigned int maxconnects. Also, I've tested it, 4294967295 works, 4294967296 doesn't any more, so it's definitely unsigned. The test fails otherwise.
| @@ -53,7 +53,7 @@ public function __construct(int $maxHostConnections, int $maxPendingPushes) | |||
| curl_multi_setopt($this->handle, \CURLMOPT_PIPELINING, \CURLPIPE_MULTIPLEX); | |||
| } | |||
| if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS')) { | |||
There was a problem hiding this comment.
let's keep the defaults when maxHostConnections is not set?
| if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS')) { | |
| if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS') && 0 < $maxHostConnections) { | |
| $maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, $maxHostConnections) ? 2147483647 : $maxHostConnections; |
There was a problem hiding this comment.
8fe2f2f to
4f9b5de
Compare
|
Thank you @HypeMC. |
…rekas) This PR was merged into the 6.4 branch. Discussion ---------- [HttpClient] Limit curl's connection cache size | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | #60513 | License | MIT Trying to accommodate for both #58278 and #60513 By default, $maxHostConnections is 6, so that this allows 50x6 = 300 connections max, which means one third of the typical `ulimit -n` (max open file descriptors), which is 1024. We could allow setting both options separately, but for now, as a bugfix, this might be enough. Commits ------- dfbe6c8 [HttpClient] Limit curl's connection cache size
An alternative approach for #58270, see #58270 (comment).