Skip to content

Commit a67de20

Browse files
nicolas-grekasnicolas-grekas
committed
[HttpClient] Fix sharing CurlClientState between clones of CurlHttpClient instances
1 parent f6e03d8 commit a67de20

File tree

2 files changed

+85
-45
lines changed

2 files changed

+85
-45
lines changed

‎Internal/CurlClientState.php‎

Lines changed: 55 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -37,45 +37,15 @@ final class CurlClientState extends ClientState
3737

3838
public static array $curlVersion;
3939

40-
public function __construct(int $maxHostConnections, int $maxPendingPushes)
41-
{
40+
public function __construct(
41+
private int $maxHostConnections,
42+
private int $maxPendingPushes,
43+
) {
4244
self::$curlVersion ??= curl_version();
43-
44-
$this->handle = curl_multi_init();
4545
$this->dnsCache = new DnsCache();
46-
$this->reset();
4746

48-
// Don't enable HTTP/1.1 pipelining: it forces responses to be sent in order
49-
if (\defined('CURLPIPE_MULTIPLEX')) {
50-
curl_multi_setopt($this->handle, \CURLMOPT_PIPELINING, \CURLPIPE_MULTIPLEX);
51-
}
52-
if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS') && 0 < $maxHostConnections) {
53-
$maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, $maxHostConnections) ? min(50 * $maxHostConnections, 4294967295) : $maxHostConnections;
54-
}
55-
if (\defined('CURLMOPT_MAXCONNECTS') && 0 < $maxHostConnections) {
56-
curl_multi_setopt($this->handle, \CURLMOPT_MAXCONNECTS, $maxHostConnections);
57-
}
58-
59-
// Skip configuring HTTP/2 push when it's unsupported or buggy, see https://bugs.php.net/77535
60-
if (0 >= $maxPendingPushes) {
61-
return;
62-
}
63-
64-
// HTTP/2 push crashes before curl 7.61
65-
if (!\defined('CURLMOPT_PUSHFUNCTION') || 0x073D00 > self::$curlVersion['version_number'] || !(\CURL_VERSION_HTTP2 & self::$curlVersion['features'])) {
66-
return;
67-
}
68-
69-
// Clone to prevent a circular reference
70-
$multi = clone $this;
71-
$multi->handle = null;
72-
$multi->share = null;
73-
$multi->pushedResponses = &$this->pushedResponses;
74-
$multi->logger = &$this->logger;
75-
$multi->handlesActivity = &$this->handlesActivity;
76-
$multi->openHandles = &$this->openHandles;
77-
78-
curl_multi_setopt($this->handle, \CURLMOPT_PUSHFUNCTION, static fn ($parent, $pushed, array $requestHeaders) => $multi->handlePush($parent, $pushed, $requestHeaders, $maxPendingPushes));
47+
// handle and share are initialized lazily in __get()
48+
unset($this->handle, $this->share);
7949
}
8050

8151
public function reset(): void
@@ -89,17 +59,59 @@ public function reset(): void
8959
$this->dnsCache->evictions = $this->dnsCache->evictions ?: $this->dnsCache->removals;
9060
$this->dnsCache->removals = $this->dnsCache->hostnames = [];
9161

92-
$this->share = curl_share_init();
62+
unset($this->share);
63+
}
64+
65+
public function __get(string $name): mixed
66+
{
67+
if ('share' === $name) {
68+
$this->share = curl_share_init();
69+
70+
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_DNS);
71+
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_SSL_SESSION);
9372

94-
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_DNS);
95-
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_SSL_SESSION);
73+
if (\defined('CURL_LOCK_DATA_CONNECT')) {
74+
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_CONNECT);
75+
}
9676

97-
if (\defined('CURL_LOCK_DATA_CONNECT')) {
98-
curl_share_setopt($this->share, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_CONNECT);
77+
return $this->share;
9978
}
79+
80+
if ('handle' === $name) {
81+
$this->handle = curl_multi_init();
82+
83+
// Don't enable HTTP/1.1 pipelining: it forces responses to be sent in order
84+
if (\defined('CURLPIPE_MULTIPLEX')) {
85+
curl_multi_setopt($this->handle, \CURLMOPT_PIPELINING, \CURLPIPE_MULTIPLEX);
86+
}
87+
if (\defined('CURLMOPT_MAX_HOST_CONNECTIONS') && 0 < $this->maxHostConnections) {
88+
$this->maxHostConnections = curl_multi_setopt($this->handle, \CURLMOPT_MAX_HOST_CONNECTIONS, $this->maxHostConnections) ? min(50 * $this->maxHostConnections, 4294967295) : $this->maxHostConnections;
89+
}
90+
if (\defined('CURLMOPT_MAXCONNECTS') && 0 < $this->maxHostConnections) {
91+
curl_multi_setopt($this->handle, \CURLMOPT_MAXCONNECTS, $this->maxHostConnections);
92+
}
93+
94+
// Skip configuring HTTP/2 push when it's unsupported or buggy, see https://bugs.php.net/77535
95+
if (0 < $this->maxPendingPushes && (\defined('CURLMOPT_PUSHFUNCTION') && 0x073D00 <= self::$curlVersion['version_number'] && (\CURL_VERSION_HTTP2 & self::$curlVersion['features']))) {
96+
// Clone to prevent a circular reference
97+
$multi = clone $this;
98+
$multi->handle = null;
99+
$multi->share = null;
100+
$multi->pushedResponses = &$this->pushedResponses;
101+
$multi->logger = &$this->logger;
102+
$multi->handlesActivity = &$this->handlesActivity;
103+
$multi->openHandles = &$this->openHandles;
104+
105+
curl_multi_setopt($this->handle, \CURLMOPT_PUSHFUNCTION, $multi->handlePush(...));
106+
}
107+
108+
return $this->handle;
109+
}
110+
111+
throw new \LogicException(\sprintf('Unknown property "%s" on "%s".', $name, self::class));
100112
}
101113

102-
private function handlePush($parent, $pushed, array $requestHeaders, int $maxPendingPushes): int
114+
private function handlePush($parent, $pushed, array $requestHeaders): int
103115
{
104116
$headers = [];
105117
$origin = curl_getinfo($parent, \CURLINFO_EFFECTIVE_URL);
@@ -127,7 +139,7 @@ private function handlePush($parent, $pushed, array $requestHeaders, int $maxPen
127139
return \CURL_PUSH_DENY;
128140
}
129141

130-
if ($maxPendingPushes <= \count($this->pushedResponses)) {
142+
if ($this->maxPendingPushes <= \count($this->pushedResponses)) {
131143
$fifoUrl = key($this->pushedResponses);
132144
unset($this->pushedResponses[$fifoUrl]);
133145
$this->logger?->debug(\sprintf('Evicting oldest pushed response: "%s"', $fifoUrl));

‎Tests/CurlHttpClientTest.php‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
use Symfony\Component\HttpClient\CurlHttpClient;
1515
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
16-
use Symfony\Contracts\HttpClient\HttpClientInterface;
1716

1817
/**
1918
* @requires extension curl
@@ -22,7 +21,7 @@
2221
*/
2322
class CurlHttpClientTest extends HttpClientTestCase
2423
{
25-
protected function getHttpClient(string $testCase): HttpClientInterface
24+
protected function getHttpClient(string $testCase): CurlHttpClient
2625
{
2726
if (!str_contains($testCase, 'Push')) {
2827
return new CurlHttpClient(['verify_peer' => false, 'verify_host' => false]);
@@ -55,6 +54,35 @@ public function testHandleIsReinitOnReset()
5554
self::assertNotSame($initialShareId, $clientState->share);
5655
}
5756

57+
public function testCurlClientStateIsSharedBetweenClones()
58+
{
59+
$client = $this->getHttpClient(__FUNCTION__);
60+
$cloneA = $client->withOptions(['headers' => ['Foo: bar']]);
61+
$cloneB = $client->withOptions(['headers' => ['Foo: baz']]);
62+
63+
$r = new \ReflectionProperty($client, 'multi');
64+
$state = $r->getValue($client);
65+
66+
self::assertSame($state, $r->getValue($cloneA));
67+
self::assertSame($state, $r->getValue($cloneB));
68+
}
69+
70+
public function testCurlClientStateInitializesHandlesLazily()
71+
{
72+
$client = $this->getHttpClient(__FUNCTION__);
73+
74+
$r = new \ReflectionProperty($client, 'multi');
75+
$state = $r->getValue($client);
76+
77+
self::assertFalse(isset($state->handle));
78+
self::assertFalse(isset($state->share));
79+
80+
$client->request('GET', 'http://127.0.0.1:8057/json')->getStatusCode();
81+
82+
self::assertInstanceOf(\CurlMultiHandle::class, $state->handle);
83+
self::assertInstanceOf(\CurlShareHandle::class, $state->share);
84+
}
85+
5886
public function testProcessAfterReset()
5987
{
6088
$client = $this->getHttpClient(__FUNCTION__);

0 commit comments

Comments
 (0)