[HttpClient] Lazily initialize CurlClientState #54207
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carsonbot
changed the title
nicolas-grekas
approved these changes
Mar 8, 2024
stof
reviewed
Mar 8, 2024
stof
approved these changes
Mar 8, 2024
fabpot
approved these changes
Mar 9, 2024
|
Don't we have the same issue in 5.4? |
|
@fabpot if this code is in 5.4, than probably. I'm not sure if the fixes are valid php 7.x though. |
nicolas-grekas
requested review from
lyrixx,
yceruto,
chalasr,
dunglas,
OskarStark and
xabbuh
as code owners
nicolas-grekas
force-pushed
the
lazy-curl-init
branch
from
dea3f27
to
6c568df
Compare
nicolas-grekas
force-pushed
the
lazy-curl-init
branch
from
6c568df
to
4966c75
Compare
|
Thank you @arjenm. |
This was referenced Apr 2, 2024
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_initcall 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.