Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 8 additions & 0 deletions psalm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
<errorLevel type="suppress">
<!-- These classes have been added in PHP 8.4 -->
<referencedClass name="BcMath\Number"/>
<!-- These classes have been added in PHP 8.5 -->
<referencedClass name="CurlSharePersistentHandle"/>
</errorLevel>
</UndefinedClass>
<UnusedClass>
Expand All @@ -49,6 +51,12 @@
<directory name="src/Symfony" />
</errorLevel>
</UnusedConstructor>
<UndefinedFunction>
<errorLevel type="suppress">
<!-- These functions have been added in PHP 8.5 -->
<referencedFunction name="curl_share_init_persistent"/>
</errorLevel>
</UndefinedFunction>
</issueHandlers>

<forbiddenFunctions>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1961,6 +1961,10 @@ private function addHttpClientSection(ArrayNodeDefinition $rootNode, callable $e
->scalarNode('http_version')
->info('The default HTTP version, typically 1.1 or 2.0, leave to null for the best version.')
->end()
->booleanNode('allow_persistent_handles')
Copy link
Member

Choose a reason for hiding this comment

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

this should be at the same level as max_host_connections, this cannot be configured as a client option
about the naming, what about this?

Suggested change
->booleanNode('allow_persistent_handles')
->booleanNode('use_persistent_connections')

->info('Whether to allow persistent handles for cURL introduced PHP 8.5.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->info('Whether to allow persistent handles for cURL introduced PHP 8.5.')
->info('Whether to share open connections, DNS and SSL state between separate Kernel requests.')

->defaultFalse()
->end()
->arrayNode('resolve')
->info('Associative array: domain => IP.')
->useAttributeAsKey('host')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
'http_client' => [
'default_options' => [
'headers' => ['X-powered' => 'PHP'],
'allow_persistent_handles' => false,
'max_redirects' => 2,
'http_version' => '2.0',
'resolve' => ['localhost' => '127.0.0.1'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ framework:
default_options:
headers:
X-powered: PHP
allow_persistent_handles: false
max_redirects: 2
http_version: 2.0
resolve: {'localhost': '127.0.0.1'}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2208,6 +2208,7 @@ public function testHttpClientDefaultOptions()

$defaultOptions = [
'headers' => [],
'allow_persistent_handles' => false,
'resolve' => [],
'extra' => [],
];
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpClient/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

8.1
---

* Add option `allow_persistent_handles` to `CurlHttpClient` to control the use of persistent connections introduced in PHP 8.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Add option `allow_persistent_handles` to `CurlHttpClient` to control the use of persistent connections introduced in PHP 8.5
* Add option `allow_persistent_handles` to `CurlHttpClient` to control the use of persistent connections introduced in PHP 8.5


8.0
---

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/HttpClient/CurlHttpClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface,

private array $defaultOptions = self::OPTIONS_DEFAULTS + [
'auth_ntlm' => null, // array|string - an array containing the username as first value, and optionally the
// password as the second one; or string like username:password - enabling NTLM auth
// password as the second one; or string like username:password - enabling NTLM auth
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// password as the second one; or string like username:password - enabling NTLM auth
// password as the second one; or string like username:password - enabling NTLM auth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this reformatting was suggested by php-cs-fixer, not by me

Copy link
Member

Choose a reason for hiding this comment

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

and we're the ones able to spot false-positives ;)

'extra' => [
'curl' => [], // A list of extra curl options indexed by their corresponding CURLOPT_*
],
'allow_persistent_handles' => false,
];
private static array $emptyDefaults = self::OPTIONS_DEFAULTS + ['auth_ntlm' => null];

Expand Down Expand Up @@ -75,7 +76,7 @@ public function __construct(array $defaultOptions = [], int $maxHostConnections
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);
}

$this->multi = new CurlClientState($maxHostConnections, $maxPendingPushes);
$this->multi = new CurlClientState($maxHostConnections, $maxPendingPushes, $defaultOptions['allow_persistent_handles'] ?? false);
}

public function setLogger(LoggerInterface $logger): void
Expand Down
28 changes: 20 additions & 8 deletions src/Symfony/Component/HttpClient/Internal/CurlClientState.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
final class CurlClientState extends ClientState
{
public ?\CurlMultiHandle $handle;
public ?\CurlShareHandle $share;
public \CurlShareHandle|\CurlSharePersistentHandle|null $share;
public bool $performing = false;

/** @var PushedResponse[] */
Expand All @@ -40,12 +40,18 @@ final class CurlClientState extends ClientState
public function __construct(
private int $maxHostConnections,
private int $maxPendingPushes,
private bool $allowPersistentHandles = false,
) {
self::$curlVersion ??= curl_version();
$this->dnsCache = new DnsCache();

// handle and share are initialized lazily in __get()
unset($this->handle, $this->share);

if ($this->allowPersistentHandles && \PHP_VERSION_ID < 80500) {
Copy link
Member

Choose a reason for hiding this comment

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

not sure this is useful
the flag is useful on PHP 8.4 also, to skip the unset in reset

$this->allowPersistentHandles = false;
@trigger_error('Upgrade PHP to version 8.5 to enable persistent handles.', \E_USER_WARNING);
}
}

public function reset(): void
Expand All @@ -59,18 +65,24 @@ public function reset(): void
$this->dnsCache->evictions = $this->dnsCache->evictions ?: $this->dnsCache->removals;
$this->dnsCache->removals = $this->dnsCache->hostnames = [];

unset($this->share);
if (!$this->allowPersistentHandles) {
unset($this->share);
}
}

public function __get(string $name): mixed
{
if ('share' === $name) {
$this->share = curl_share_init();

curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_DNS);
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_SSL_SESSION);

if (\defined('CURL_LOCK_DATA_CONNECT')) {
if ($this->allowPersistentHandles && (\PHP_VERSION_ID >= 80500)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($this->allowPersistentHandles && (\PHP_VERSION_ID >= 80500)) {
if (\PHP_VERSION_ID >= 80500 && $this->allowPersistentHandles) {

$this->share = curl_share_init_persistent([
\CURL_LOCK_DATA_DNS,
\CURL_LOCK_DATA_SSL_SESSION,
\CURL_LOCK_DATA_CONNECT,
]);
} else {
$this->share = curl_share_init();
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_DNS);
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_SSL_SESSION);
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_CONNECT);
}

Expand Down
23 changes: 21 additions & 2 deletions src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\HttpClient\Tests;

use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\Attributes\RequiresPhp;
use PHPUnit\Framework\Attributes\RequiresPhpExtension;
use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
Expand All @@ -22,15 +23,16 @@ class CurlHttpClientTest extends HttpClientTestCase
{
protected function getHttpClient(string $testCase): CurlHttpClient
{
$allowPersistentHandles = str_contains($testCase, 'Persistent');
if (!str_contains($testCase, 'Push')) {
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false]);
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false, 'allow_persistent_handles' => $allowPersistentHandles]);
}

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, 'allow_persistent_handles' => $allowPersistentHandles], 6, 50);
}

public function testTimeoutIsNotAFatalError()
Expand Down Expand Up @@ -82,6 +84,23 @@ public function testCurlClientStateInitializesHandlesLazily()
self::assertInstanceOf(\CurlShareHandle::class, $state->share);
}

#[RequiresPhp('>= 8.5')]
public function testCurlClientPersistentStateInitializesHandlesLazily()
{
$client = $this->getHttpClient(__FUNCTION__);

$r = new \ReflectionProperty($client, 'multi');
$state = $r->getValue($client);

self::assertFalse(isset($state->handle));
self::assertFalse(isset($state->share));

$client->request('GET', 'http://127.0.0.1:8057/json')->getStatusCode();

self::assertInstanceOf(\CurlMultiHandle::class, $state->handle);
self::assertInstanceOf(\CurlSharePersistentHandle::class, $state->share);
}

public function testProcessAfterReset()
{
$client = $this->getHttpClient(__FUNCTION__);
Expand Down
Loading