Skip to content

Commit 714a20e

Browse files
nicolas-grekasnicolas-grekas
committed
bug #62645 [HttpClient] Fix sharing CurlClientState between clones of CurlHttpClient instances (nicolas-grekas)
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpClient] Fix sharing CurlClientState between clones of CurlHttpClient instances | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #62633 | License | MIT PR #54207 fixed descriptor exhaustion for apps that never issued HTTP calls, but it inadvertently broke the sharing of Curl state between cloned/scoped clients. Any place that pre-creates lots of withOptions() variants before the first real request will now instantiate a separate CurlClientState per variant, leaking curl multi handles relative to previous releases. The fix restores state sharing so multiple clones can still point to the same CurlClientState while maintaining the “only allocate when someone actually sends a request” behavior. That's the only thing I could think of that'd explain #62633 /cc `@jwage` can you give this a try? Commits ------- 18dca38 [HttpClient] Fix sharing CurlClientState between clones of CurlHttpClient instances c0cad9e Revert "[HttpClient] Lazily initialize CurlClientState"
2 parents baf4a16 + 18dca38 commit 714a20e

File tree

3 files changed

+106
-89
lines changed

3 files changed

+106
-89
lines changed

‎src/Symfony/Component/HttpClient/CurlHttpClient.php‎

Lines changed: 19 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,6 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface,
5151

5252
private ?LoggerInterface $logger = null;
5353

54-
private int $maxHostConnections;
55-
private int $maxPendingPushes;
56-
5754
/**
5855
* An internal object to share state between the client and its responses.
5956
*/
@@ -72,31 +69,25 @@ public function __construct(array $defaultOptions = [], int $maxHostConnections
7269
throw new \LogicException('You cannot use the "Symfony\Component\HttpClient\CurlHttpClient" as the "curl" extension is not installed.');
7370
}
7471

75-
$this->maxHostConnections = $maxHostConnections;
76-
$this->maxPendingPushes = $maxPendingPushes;
77-
7872
$this->defaultOptions['buffer'] ??= self::shouldBuffer(...);
7973

8074
if ($defaultOptions) {
8175
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);
8276
}
77+
78+
$this->multi = new CurlClientState($maxHostConnections, $maxPendingPushes);
8379
}
8480

8581
public function setLogger(LoggerInterface $logger): void
8682
{
87-
$this->logger = $logger;
88-
if (isset($this->multi)) {
89-
$this->multi->logger = $logger;
90-
}
83+
$this->logger = $this->multi->logger = $logger;
9184
}
9285

9386
/**
9487
* @see HttpClientInterface::OPTIONS_DEFAULTS for available options
9588
*/
9689
public function request(string $method, string $url, array $options = []): ResponseInterface
9790
{
98-
$multi = $this->ensureState();
99-
10091
[$url, $options] = self::prepareRequest($method, $url, $options, $this->defaultOptions);
10192
$scheme = $url['scheme'];
10293
$authority = $url['authority'];
@@ -174,24 +165,24 @@ public function request(string $method, string $url, array $options = []): Respo
174165
}
175166

176167
// curl's resolve feature varies by host:port but ours varies by host only, let's handle this with our own DNS map
177-
if (isset($multi->dnsCache->hostnames[$host])) {
178-
$options['resolve'] += [$host => $multi->dnsCache->hostnames[$host]];
168+
if (isset($this->multi->dnsCache->hostnames[$host])) {
169+
$options['resolve'] += [$host => $this->multi->dnsCache->hostnames[$host]];
179170
}
180171

181-
if ($options['resolve'] || $multi->dnsCache->evictions) {
172+
if ($options['resolve'] || $this->multi->dnsCache->evictions) {
182173
// First reset any old DNS cache entries then add the new ones
183-
$resolve = $multi->dnsCache->evictions;
184-
$multi->dnsCache->evictions = [];
174+
$resolve = $this->multi->dnsCache->evictions;
175+
$this->multi->dnsCache->evictions = [];
185176

186177
if ($resolve && 0x072A00 > CurlClientState::$curlVersion['version_number']) {
187178
// DNS cache removals require curl 7.42 or higher
188-
$multi->reset();
179+
$this->multi->reset();
189180
}
190181

191182
foreach ($options['resolve'] as $resolveHost => $ip) {
192183
$resolve[] = null === $ip ? "-$resolveHost:$port" : "$resolveHost:$port:$ip";
193-
$multi->dnsCache->hostnames[$resolveHost] = $ip;
194-
$multi->dnsCache->removals["-$resolveHost:$port"] = "-$resolveHost:$port";
184+
$this->multi->dnsCache->hostnames[$resolveHost] = $ip;
185+
$this->multi->dnsCache->removals["-$resolveHost:$port"] = "-$resolveHost:$port";
195186
}
196187

197188
$curlopts[\CURLOPT_RESOLVE] = $resolve;
@@ -293,16 +284,16 @@ public function request(string $method, string $url, array $options = []): Respo
293284
$curlopts += $options['extra']['curl'];
294285
}
295286

296-
if ($pushedResponse = $multi->pushedResponses[$url] ?? null) {
297-
unset($multi->pushedResponses[$url]);
287+
if ($pushedResponse = $this->multi->pushedResponses[$url] ?? null) {
288+
unset($this->multi->pushedResponses[$url]);
298289

299290
if (self::acceptPushForRequest($method, $options, $pushedResponse)) {
300291
$this->logger?->debug(\sprintf('Accepting pushed response: "%s %s"', $method, $url));
301292

302293
// Reinitialize the pushed response with request's options
303294
$ch = $pushedResponse->handle;
304295
$pushedResponse = $pushedResponse->response;
305-
$pushedResponse->__construct($multi, $url, $options, $this->logger);
296+
$pushedResponse->__construct($this->multi, $url, $options, $this->logger);
306297
} else {
307298
$this->logger?->debug(\sprintf('Rejecting pushed response: "%s"', $url));
308299
$pushedResponse = null;
@@ -312,7 +303,7 @@ public function request(string $method, string $url, array $options = []): Respo
312303
if (!$pushedResponse) {
313304
$ch = curl_init();
314305
$this->logger?->info(\sprintf('Request: "%s %s"', $method, $url));
315-
$curlopts += [\CURLOPT_SHARE => $multi->share];
306+
$curlopts += [\CURLOPT_SHARE => $this->multi->share];
316307
}
317308

318309
foreach ($curlopts as $opt => $value) {
@@ -325,7 +316,7 @@ public function request(string $method, string $url, array $options = []): Respo
325316
}
326317
}
327318

328-
return $pushedResponse ?? new CurlResponse($multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $authority), CurlClientState::$curlVersion['version_number'], $url);
319+
return $pushedResponse ?? new CurlResponse($this->multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $authority), CurlClientState::$curlVersion['version_number'], $url);
329320
}
330321

331322
public function stream(ResponseInterface|iterable $responses, ?float $timeout = null): ResponseStreamInterface
@@ -334,11 +325,9 @@ public function stream(ResponseInterface|iterable $responses, ?float $timeout =
334325
$responses = [$responses];
335326
}
336327

337-
$multi = $this->ensureState();
338-
339-
if ($multi->handle instanceof \CurlMultiHandle) {
328+
if ($this->multi->handle instanceof \CurlMultiHandle) {
340329
$active = 0;
341-
while (\CURLM_CALL_MULTI_PERFORM === curl_multi_exec($multi->handle, $active)) {
330+
while (\CURLM_CALL_MULTI_PERFORM === curl_multi_exec($this->multi->handle, $active)) {
342331
}
343332
}
344333

@@ -347,9 +336,7 @@ public function stream(ResponseInterface|iterable $responses, ?float $timeout =
347336

348337
public function reset(): void
349338
{
350-
if (isset($this->multi)) {
351-
$this->multi->reset();
352-
}
339+
$this->multi->reset();
353340
}
354341

355342
/**
@@ -448,16 +435,6 @@ private static function createRedirectResolver(array $options, string $authority
448435
};
449436
}
450437

451-
private function ensureState(): CurlClientState
452-
{
453-
if (!isset($this->multi)) {
454-
$this->multi = new CurlClientState($this->maxHostConnections, $this->maxPendingPushes);
455-
$this->multi->logger = $this->logger;
456-
}
457-
458-
return $this->multi;
459-
}
460-
461438
private function findConstantName(int $opt): ?string
462439
{
463440
$constants = array_filter(get_defined_constants(), static fn ($v, $k) => $v === $opt && 'C' === $k[0] && (str_starts_with($k, 'CURLOPT_') || str_starts_with($k, 'CURLINFO_')), \ARRAY_FILTER_USE_BOTH);

‎src/Symfony/Component/HttpClient/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));

‎src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php‎

Lines changed: 32 additions & 4 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]);
@@ -48,13 +47,42 @@ public function testHandleIsReinitOnReset()
4847
{
4948
$httpClient = $this->getHttpClient(__FUNCTION__);
5049

51-
$r = new \ReflectionMethod($httpClient, 'ensureState');
52-
$clientState = $r->invoke($httpClient);
50+
$r = new \ReflectionProperty($httpClient, 'multi');
51+
$clientState = $r->getValue($httpClient);
5352
$initialShareId = $clientState->share;
5453
$httpClient->reset();
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)