Hello again internals,
Similar to the Random Extension improvement RFCPHP: rfc:random_extension_improvement ), I am creatingPHP: rfc:curl_share_persistence ).
https://wiki.php.net/rfc/curl_share_persistence_improvement
After making changes based on feedback in the original implementationfeat: enable persistent `CurlShareHandle` objects by ericnorris · Pull Request #15603 · php/php-src · GitHub ), I had some
php:master ← ericnorris:feat-persistent-curl-share-handle-alt
opened 08:01PM - 25 Nov 24 UTC
This is an alternative to #15603, and both are relevant to https://wiki.php.net/… rfc/curl_share_persistence.
During the voting period, @TimWolla provided some feedback on the internals mailing list:
> Also badly chosen `$persistent_id`s might result in a
> large number of handles accumulating, without any kind of garbage
> collection. For the existing persistent handles (e.g. for database
> connections), the ID is chosen by PHP itself, ensuring a somewhat
> predictable behavior.
and:
> Given the information regarding the TLS re-use, the cookie sharing is my
> only remaining concern. In fact with cookie sharing disabled it might
> not even be necessary for the user to choose an ID: Given that libcurl
> does the heavy lifting, as a user I should only need a single share
> handle and let libcurl figure out the details, no? A boolean “share
> connections across requests” when initializing a CurlHandle should
> probably sufficient.
After fixing up the original PR, I had some free time this past weekend to explore an alternative implementation that addressed these points.
In this implementation I've introduced a new function, `curl_persistent_share_init(array $share_options)`, which returns a `CurlPersistentShareHandle` object. Under the hood, the object is identical to a `CurlShareHandle`, and can be used in `curl_setopt`. It cannot be used with `curl_share_setopt`, however, which ensures that the handle is immutable across requests - something the other PR could not do in a statically analyzable way.
The function uses the share options to construct a persistent ID internally, thus eliminating the need for a user to pick the persistent ID. Users will, however, need to ensure [`CURLOPT_MAXCONNECTS`](https://www.php.net/manual/en/curl.constants.php#constant.curlopt-maxconnects) is set appropriately on a `CurlHandle`, since there will only be a single share handle per unique set of `$share_options`. Note that this was true in the other PR, but less important since you could pick a unique persistent ID.
The function disallows `CURL_LOCK_DATA_COOKIE`; I don't particularly feel strongly about this but I think it's easier to relax restrictions in the future compared to making them more strict.
The second commit introduces a read-only property to allow users to introspect the options set on a persistent share handle, but I could remove this commit if people felt it was unnecessary.
I'm looking for the following feedback:
- Is this better than and preferred to #15603?
- In the second commit, I've exposed `php_array_data_compare_unstable_i` in order to avoid having to duplicate comparison logic. I see that [`collator_sort.c`](https://github.com/php/php-src/blob/aa05c827e2984ab41df45b05406d82d6872678ff/ext/intl/collator/collator_sort.c#L188-L219) had a similar problem, but took the path of duplicating the logic. What is preferred?
- Is the "hash" function I used to create a unique persistent ID acceptable?
The result, I think, is actually an improvement over the original.
The RFC has two voting choices, the first is to accept the new
I plan on opening the vote in two weeks barring any major discussion.
Thanks for your time and the feedback so far!
Hi
On 11/27/24 18:13, Eric Norris wrote:
PHP: rfc:curl_share_persistence_improvement
Thank you. As I've already said in the PR, this is a clear improvement that resolves my concerns with the initially proposed API. I'll happily vote in favor of this follow-up RFC and am looking forward to the usecases this enables.
Best regards