8000 bug #54207 [HttpClient] Lazily initialize CurlClientState (arjenm) · symfony/symfony@0a7825b · GitHub
[go: up one dir, main page]
Skip to content

Commit 0a7825b

Browse files
nicolas-grekasnicolas-grekas
committed
bug #54207 [HttpClient] Lazily initialize CurlClientState (arjenm)
This PR was merged into the 5.4 branch. Discussion ---------- [HttpClient] Lazily initialize CurlClientState | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | See below | License | MIT We have a dependency that recently started requiring symfony/http-client. Once that happened, our testing pipeline started to fail. The failure turned out to be running out of open file descriptors. It took a while to pinpoint the cause, but we identified the constructor of CurlClientState, specifically the `curl_multi_init` call in it, to cause an ever increasing number of open files (specifically, unix streams). In our platform, we don't actually use HttpClient. But several services in Symfony have an "optional" dependency on it. Our examples were the TransportFactory-classes in the Mailer-package (including that for the NullTransport used in our testing pipeline) and the JsonManifestVersionStrategy. I.e. as soon as the http-client package was installed, any time those services where requested, the container now first had to create the HttpClient-service. Which then selected CurlHttpClient as the best option. Which constructed its CurlClientState. And that in turn preemptively opened that stream. We trigger the Mailer-service for every test, and given our 11k+ tests, that happened a lot of times. And apparently, the streams didn't get closed properly since we ran out of file descriptors after something like 9k tests done... Unfortunately I was unable to create an isolated reproducer. My guess is that the reproducer was so much smaller, that PHP's memory cleanup could keep up and quickly identify and close abandoned resources (like the curl multihandle), whereas it could not in our much bigger application. But the issue (lots of 'STREAM' rows in the output of netstat or lsof) did not appear when an 'early return' was added to the CurlClientState's constructor (or when we explicitly specified null as httpclient for the mailer and our JsonManifestVersionStrategy). This PR delays the (imo very heavy) construction of CurlClientState (including the initial reset) in CurlHttpClient to be a "just in time" occurrence. Commits ------- 4966c75 [HttpClient] Lazily initialize CurlClientState
2 parents c6506c4 + 4966c75 commit 0a7825b

File tree

2 files changed

+44
-21
lines changed

2 files changed

+44
-21
lines changed

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

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface,
5050
*/
5151
private $logger;
5252

53+
private $maxHostConnections;
54+
private $maxPendingPushes;
55+
5356
/**
5457
* An internal object to share state between the client and its responses.
5558
*
@@ -70,18 +73,22 @@ public function __construct(array $defaultOptions = [], int $maxHostConnections
7073
throw new \LogicException('You cannot use the "Symfony\Component\HttpClient\CurlHttpClient" as the "curl" extension is not installed.');
7174
}
7275

76+
$this->maxHostConnections = $maxHostConnections;
77+
$this->maxPendingPushes = $maxPendingPushes;
78+
7379
$this->defaultOptions['buffer'] = $this->defaultOptions['buffer'] ?? \Closure::fromCallable([__CLASS__, 'shouldBuffer']);
7480

7581
if ($defaultOptions) {
7682
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);
7783
}
78-
79-
$this->multi = new CurlClientState($maxHostConnections, $maxPendingPushes);
8084
}
8185

8286
public function setLogger(LoggerInterface $logger): void
8387
{
84-
$this->logger = $this->multi->logger = $logger;
88+
$this->logger = $logger;
89+
if (isset($this->multi)) {
90+
$this->multi->logger = $logger;
91+
}
8592
}
8693

8794
/**
@@ -91,6 +98,8 @@ public function setLogger(LoggerInterface $logger): void
9198
*/
9299
public function request(string $method, string $url, array $options = []): ResponseInterface
93100
{
101+
$multi = $this->ensureState();
102+
94103
[$url, $options] = self::prepareRequest($method, $url, $options, $this->defaultOptions);
95104
$scheme = $url['scheme'];
96105
$authority = $url['authority'];
@@ -161,25 +170,25 @@ public function request(string $method, string $url, array $options = []): Respo
161170
}
162171

163172
// curl's resolve feature varies by host:port but ours varies by host only, let's handle this with our own DNS map
164-
if (isset($this->multi->dnsCache->hostnames[$host])) {
165-
$options['resolve'] += [$host => $this->multi->dnsCache->hostnames[$host]];
173+
if (isset($multi->dnsCache->hostnames[$host])) {
174+
$options['resolve'] += [$host => $multi->dnsCache->hostnames[$host]];
166175
}
167176

168-
if ($options['resolve'] || $this->multi->dnsCache->evictions) {
177+
if ($options['resolve'] || $multi->dnsCache->evictions) {
169178
// First reset any old DNS cache entries then add the new ones
170-
$resolve = $this->multi->dnsCache->evictions;
171-
$this->multi->dnsCache->evictions = [];
179+
$resolve = $multi->dnsCache->evictions;
180+
$multi->dnsCache->evictions = [];
172181
$port = parse_url($authority, \PHP_URL_PORT) ?: ('http:' === $scheme ? 80 : 443);
173182

174183
if ($resolve && 0x072A00 > CurlClientState::$curlVersion['version_number']) {
175184
// DNS cache removals require curl 7.42 or higher
176-
$this->multi->reset();
185+
$multi->reset();
177186
}
178187

179188
foreach ($options['resolve'] as $host => $ip) {
180189
$resolve[] = null === $ip ? "-$host:$port" : "$host:$port:$ip";
181-
$this->multi->dnsCache->hostnames[$host] = $ip;
182-
$this->multi->dnsCache->removals["-$host:$port"] = "-$host:$port";
190+
$multi->dnsCache->hostnames[$host] = $ip;
191+
$multi->dnsCache->removals["-$host:$port"] = "-$host:$port";
183192
}
184193

185194
$curlopts[\CURLOPT_RESOLVE] = $resolve;
@@ -281,16 +290,16 @@ public function request(string $method, string $url, array $options = []): Respo
281290
$curlopts += $options['extra']['curl'];
282291
}
283292

284-
if ($pushedResponse = $this->multi->pushedResponses[$url] ?? null) {
285-
unset($this->multi->pushedResponses[$url]);
293+
if ($pushedResponse = $multi->pushedResponses[$url] ?? null) {
294+
unset($multi->pushedResponses[$url]);
286295

287296
if (self::acceptPushForRequest($method, $options, $pushedResponse)) {
288297
$this->logger && $this->logger->debug(sprintf('Accepting pushed response: "%s %s"', $method, $url));
289298

290299
// Reinitialize the pushed response with request's options
291300
$ch = $pushedResponse->handle;
292301
$pushedResponse = $pushedResponse->response;
293-
$pushedResponse->__construct($this->multi, $url, $options, $this->logger);
302+
$pushedResponse->__construct($multi, $url, $options, $this->logger);
294303
} else {
295304
$this->logger && $this->logger->debug(sprintf('Rejecting pushed response: "%s"', $url));
296305
$pushedResponse = null;
@@ -300,7 +309,7 @@ public function request(string $method, string $url, array $options = []): Respo
300309
if (!$pushedResponse) {
301310
$ch = curl_init();
302311
$this->logger && $this->logger->info(sprintf('Request: "%s %s"', $method, $url));
303-
$curlopts += [\CURLOPT_SHARE => $this->multi->share];
312+
$curlopts += [\CURLOPT_SHARE => $multi->share];
304313
}
305314

306315
foreach ($curlopts as $opt => $value) {
@@ -310,7 +319,7 @@ public function request(string $method, string $url, array $options = []): Respo
310319
}
311320
}
312321

313-
return $pushedResponse ?? new CurlResponse($this->multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $host), CurlClientState::$curlVersion['version_number']);
322+
return $pushedResponse ?? new CurlResponse($multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $host), CurlClientState::$curlVersion['version_number']);
314323
}
315324

316325
/**
@@ -324,9 +333,11 @@ public function stream($responses, ?float $timeout = null): ResponseStreamInterf
324333
throw new \TypeError(sprintf('"%s()" expects parameter 1 to be an iterable of CurlResponse objects, "%s" given.', __METHOD__, get_debug_type($responses)));
325334
}
326335

327-
if (\is_resource($this->multi->handle) || $this->multi->handle instanceof \CurlMultiHandle) {
336+
$multi = $this->ensureState();
337+
338+
if (\is_resource($multi->handle) || $multi->handle instanceof \CurlMultiHandle) {
328339
$active = 0;
329-
while (\CURLM_CALL_MULTI_PERFORM === curl_multi_exec($this->multi->handle, $active)) {
340+
while (\CURLM_CALL_MULTI_PERFORM === curl_multi_exec($multi->handle, $active)) {
330341
}
331342
}
332343

@@ -335,7 +346,9 @@ public function stream($responses, ?float $timeout = null): ResponseStreamInterf
335346

336347
public function reset()
337348
{
338-
$this->multi->reset();
349+
if (isset($this->multi)) {
350+
$this->multi->reset();
351+
}
339352
}
340353

341354
/**
@@ -439,6 +452,16 @@ private static function createRedirectResolver(array $options, string $host): \C
439452
};
440453
}
441454

455+
private function ensureState(): CurlClientState
456+
{
457+
if (!isset($this->multi)) {
458+
$this->multi = new CurlClientState($this->maxHostConnections, $this->maxPendingPushes);
459+
$this->multi->logger = $this->logger;
460+
}
461+
462+
return $this->multi;
463+
}
464+
442465
private function findConstantName(int $opt): ?string
443466
{
444467
$constants = array_filter(get_defined_constants(), static function ($v, $k) use ($opt) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,9 @@ public function testHandleIsReinitOnReset()
6363
{
6464
$httpClient = $this->getHttpClient(__FUNCTION__);
6565

66-
$r = new \ReflectionProperty($httpClient, 'multi');
66+
$r = new \ReflectionMethod($httpClient, 'ensureState');
6767
$r->setAccessible(true);
68-
$clientState = $r->getValue($httpClient);
68+
$clientState = $r->invoke($httpClient);
6969
$initialShareId = $clientState->share;
7070
$httpClient->reset();
7171
self::assertNotSame($initialShareId, $clientState->share);

0 commit comments

Comments
 (0)
0