[HttpClient] Add support of the persistent cURL handles #62751
Conversation
2482166 to
8e06a61
Compare
There was a problem hiding this comment.
Doing this conflicts with the reset method.
It'd be strange to share the curl state between FPM processes but also reset it between requests.
The patch in this PR should remove the unset($this->share); to be consistent.
Then, we should discuss about the benefit vs risk of the shared vs insulate approach.
I don't have data about either so I'm open to more feedback.
8e06a61 to
aaed9f8
Compare
|
@nicolas-grekas nice catch. I've removed In our current project we do a lot of http requests. And there are too many of them has slow ssl handshake (up to 100ms). By introducton of this change we can reduce latency a lot. There is a good video from @beberlei about that |
|
I'm sure it can help reduce latency. I'm just wondering if it could bring some instabilities or unwanted state sharing. /cc @jwage it'd be great if you could check this one to see how this behaves with your load! |
aaed9f8 to
4736d12
Compare
Ohh I was just thinking about this exact thing this morning. So this allows us to share connections between php-fpm requests? I'm still on 8.3 and we are upgrading to 8.4 but as soon as we get to 8.5 I will test this. I was considering proxying all http-client traffic through a sidecar proxy like envoy to get persistent connections but this would be much simpler! Is it possible to make it opt in somehow so that we can introduce it safely and then if we have no issues, later we can change it to be enabled by default? |
7fcb1a8 to
8f1c2e2
Compare
8f1c2e2 to
14de47a
Compare
|
@nicolas-grekas @jwage I've added configuration option (disabled by default) to enable new behavior. Please check |
| 8.1 | ||
| --- | ||
|
|
||
| * Add option `allow_persistent_handles` to `CurlHttpClient` to control the use of persistent connections introduced in PHP 8.5 |
There was a problem hiding this comment.
| * 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 |
| 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 |
There was a problem hiding this comment.
| // 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 |
There was a problem hiding this comment.
this reformatting was suggested by php-cs-fixer, not by me
There was a problem hiding this comment.
and we're the ones able to spot false-positives ;)
|
So when I enable "allow", is it used or does cURL decides when to use them? Otherwise the term "allow" is misleading |
| ->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') |
There was a problem hiding this comment.
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?
| ->booleanNode('allow_persistent_handles') | |
| ->booleanNode('use_persistent_connections') |
| ->info('The default HTTP version, typically 1.1 or 2.0, leave to null for the best version.') | ||
| ->end() | ||
| ->booleanNode('allow_persistent_handles') | ||
| ->info('Whether to allow persistent handles for cURL introduced PHP 8.5.') |
There was a problem hiding this comment.
| ->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.') |
| // handle and share are initialized lazily in __get() | ||
| unset($this->handle, $this->share); | ||
|
|
||
| if ($this->allowPersistentHandles && \PHP_VERSION_ID < 80500) { |
There was a problem hiding this comment.
not sure this is useful
the flag is useful on PHP 8.4 also, to skip the unset in reset
| 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)) { |
There was a problem hiding this comment.
| if ($this->allowPersistentHandles && (\PHP_VERSION_ID >= 80500)) { | |
| if (\PHP_VERSION_ID >= 80500 && $this->allowPersistentHandles) { |
PHP 8.5 adds support of the persistent cURL handles which can improve performance for HTTP requests.