Skip to content

[HttpClient] Allow setting max persistent connections in CurlHttpClient #58270

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[HttpClient] Allow setting max persistent connections in CurlHttpClient #58270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/integration-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,17 @@ jobs:
image: dunglas/frankenphp:1.1.0
ports:
- 80:80
- 8081:81
- 8082:82
volumes:
- ${{ github.workspace }}:/symfony
env:
SERVER_NAME: 'http://localhost'
SERVER_NAME: 'http://localhost http://localhost:81 http://localhost:82'
CADDY_SERVER_EXTRA_DIRECTIVES: |
route /http-client* {
root * /symfony/src/Symfony/Component/HttpClient/Tests/Fixtures/response-functional/
php_server
}
Comment on lines -128 to +135
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PHP built-in server doesn't support persistent connections.

root * /symfony/src/Symfony/Component/HttpFoundation/Tests/Fixtures/response-functional/

steps:
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpClient/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
---

* Add support for amphp/http-client v5 on PHP 8.4+
* Allow setting max persistent connections in `CurlHttpClient`

7.1
---
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/HttpClient/CurlHttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface,
// password as the second one; or string like username:password - enabling NTLM auth
'extra' => [
'curl' => [], // A list of extra curl options indexed by their corresponding CURLOPT_*
'max_connections' => null, // The maximum amount of simultaneously open connections, corresponds to CURLMOPT_MAXCONNECTS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't put it under curl as this is a curl multi-handler option.

],
];
private static array $emptyDefaults = self::OPTIONS_DEFAULTS + ['auth_ntlm' => null];
Expand Down Expand Up @@ -450,7 +451,7 @@ private static function createRedirectResolver(array $options, string $host, int
private function ensureState(): CurlClientState
{
if (!isset($this->multi)) {
$this->multi = new CurlClientState($this->maxHostConnections, $this->maxPendingPushes);
$this->multi = new CurlClientState($this->maxHostConnections, $this->maxPendingPushes, $this->defaultOptions['extra']['max_connections'] ?? null);
$this->multi->logger = $this->logger;
}

Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/HttpClient/Internal/CurlClientState.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ final class CurlClientState extends ClientState

public static array $curlVersion;

public function __construct(int $maxHostConnections, int $maxPendingPushes)
public function __construct(int $maxHostConnections, int $maxPendingPushes, ?int $maxConnections = null)
{
self::$curlVersion ??= curl_version();

Expand All @@ -52,8 +52,8 @@ public function __construct(int $maxHostConnections, int $maxPendingPushes)
if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS')) {
$maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, 0 < $maxHostConnections ? $maxHostConnections : \PHP_INT_MAX) ? 0 : $maxHostConnections;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 0 is the issue. It's not documented as meaning "infinite", unlike many other curl options.

Suggested change
$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) ? \PHP_INT_MAX : $maxHostConnections;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas Yes, you're right, see #58278. I opened it as a bug fix, if it's not, please let me know.

}
if (\defined('CURLMOPT_MAXCONNECTS') && 0 < $maxHostConnections) {
curl_multi_setopt($this->handle, \CURLMOPT_MAXCONNECTS, $maxHostConnections);
if (\defined('CURLMOPT_MAXCONNECTS') && null !== $maxConnections ??= 0 < $maxHostConnections ? $maxHostConnections : null) {
curl_multi_setopt($this->handle, \CURLMOPT_MAXCONNECTS, $maxConnections);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding a new option, can you try removing this line and see if curl doesn't do better by default?

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I re-read your description, you tell about it already.

Then what about setting $maxHostConnections to 500?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas Unfortunately, setting $maxHostConnections only sets CURLMOPT_MAX_HOST_CONNECTIONS. This doesn't change CURLMOPT_MAXCONNECTS which by default is 4 times the number of added handlers. I've already tried this, and it didn't work.

}

// Skip configuring HTTP/2 push when it's unsupported or buggy, see https://bugs.php.net/77535
Expand Down
60 changes: 57 additions & 3 deletions src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,17 @@
*/
class CurlHttpClientTest extends HttpClientTestCase
{
protected function getHttpClient(string $testCase): HttpClientInterface
protected function getHttpClient(string $testCase, array $options = []): HttpClientInterface
{
if (!str_contains($testCase, 'Push')) {
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false]);
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false] + $options);
}

if (!\defined('CURLMOPT_PUSHFUNCTION') || 0x073D00 > ($v = curl_version())['version_number'] || !(\CURL_VERSION_HTTP2 & $v['features'])) {
$this->markTestSkipped('curl <7.61 is used or it is not compiled with support for HTTP/2 PUSH');
}

return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false], 6, 50);
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false] + $options, 6, 50);
}

public function testBindToPort()
Expand Down Expand Up @@ -138,4 +138,58 @@ public function testKeepAuthorizationHeaderOnRedirectToSameHostWithConfiguredHos
$this->assertSame(200, $response->getStatusCode());
$this->assertSame('/302', $response->toArray()['REQUEST_URI'] ?? null);
}

/**
* @group integration
*
* @dataProvider provideMaxConnectionCases
*/
public function testMaxConnections(?int $maxConnections, array $expectedResults)
{
foreach ($ports = [80, 8081, 8082] as $port) {
if (!($fp = @fsockopen('localhost', $port, $errorCode, $errorMessage, 2))) {
self::markTestSkipped('FrankenPHP is not running');
}
fclose($fp);
}

$httpClient = $this->getHttpClient(__FUNCTION__, ['extra' => ['max_connections' => $maxConnections]]);

foreach ($expectedResults as $expectedResult) {
foreach ($ports as $i => $port) {
$response = $httpClient->request('GET', \sprintf('http://localhost:%s/http-client', $port));
$response->getContent();

self::assertSame($expectedResult[$i], str_contains($response->getInfo('debug'), 'Re-using existing connection'));
}
}
}

public static function provideMaxConnectionCases(): iterable
{
yield 'default' => [
null,
[
[false, false, false],
[true, true, true],
[true, true, true],
],
];
yield 'one' => [
1,
[
[false, false, false],
[false, false, false],
[false, false, false],
],
];
yield 'exact' => [
3,
[
[false, false, false],
[true, true, true],
[true, true, true],
],
];
}
}
12 changes: 12 additions & 0 deletions src/Symfony/Component/HttpClient/Tests/Fixtures/response-functional/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

echo 'Success';
Loading