RFC: Add CurlSharePersistentHandle objects

[ericnorris]

RFC: https://wiki.php.net/rfc/curl_share_persistence_improvement

---

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_ids 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_share_persistent_init(array $share_options), which returns a CurlSharePersistentHandle 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 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 feat: enable persistent CurlShareHandle objects #15603? The RFC for this PR was accepted, so this supersedes the prior PR.
• 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 had a similar problem, but took the path of duplicating the logic. What is preferred?. I've tried a different approach for constructing the ->options property.
• Is the "hash" function I used to create a unique persistent ID acceptable?

[TimWolla]

Thank you. I find these changes much more agreeable. Unfortunately they are not what has been agreed on in the RFC, so we probably need a follow-up RFC, that I would happily vote in favor of.

[ericnorris]

Thank you. I find these changes much more agreeable.

Thank you for the feedback in the first place! As I look more at this implementation over the other, I do think I prefer this one.

Unfortunately they are not what has been agreed on in the RFC, so we probably need a follow-up RFC, that I would happily vote in favor of.

I agree that this might not be exactly the letter of the RFC vote, but I do think it's in the spirit - the title of the RFC is just "Add persistent curl share handles".

That said, I'm open to creating another RFC, but I'm wary of being too noisy. I would need to send out another discussion email, etc. Is there precedent for the amount of leeway in post-RFC implementation? I follow the internals mailing list, but not the implementation PRs closely enough to know the answer.

[TimWolla]

That said, I'm open to creating another RFC, but I'm wary of being too noisy. I would need to send out another discussion email, etc. Is there precedent for the amount of leeway in post-RFC implementation? I follow the internals mailing list, but not the implementation PRs closely enough to know the answer.

Your RFC was pretty specific about the proposed API (that's why I was so concerned during the discussion and would have preferred if the vote had failed). I don't think there's precedent for that amount of implementation leeway as done here and it required a mailing list email in all cases. The #[\SensitiveParameter] RFC had a change that was resolved by simple agreement, but that was a fix for behavior that clearly was non-sense: https://wiki.php.net/rfc/redact_parameters_in_back_traces#errata

In any case there's precedent for follow-up RFCs, due to issues found only after the vote started:

• https://wiki.php.net/rfc/random_extension_improvement
• https://wiki.php.net/rfc/fix_up_bcmath_number_class
• https://wiki.php.net/rfc/hook_improvements

As both a user and a maintainer, I prefer some additional RFC noise over the pain that a subpar implementation would cause in the next decade.

[Girgias]

I think this approach is vastly better than the initial implementation. So thank you!

One question about:

Users will, however, need to ensure CURLOPT_MAXCONNECTS is set appropriately on a CurlHandle, since there will only be a single share handle per unique set of $share_options

Did you mean CurlPersistentShareHandle instead of CurlHandle in that comment? And if yes, why not add a parameter to the init function int $max_connections so that one cannot forget to set this option?

[ericnorris]

Understood, @TimWolla, I'm working on writing an "improvement" RFC as we speak, and then I'll come back to this PR.

I think this approach is vastly better than the initial implementation. So thank you!

Thank you for taking a look @Girgias!

One question about:

Users will, however, need to ensure CURLOPT_MAXCONNECTS is set appropriately on a CurlHandle, since there will only be a single share handle per unique set of $share_options

Did you mean CurlPersistentShareHandle instead of CurlHandle in that comment? And if yes, why not add a parameter to the init function int $max_connections so that one cannot forget to set this option?

I believe I meant CurlHandle. It's a little unintuitive to me, but it seems like the max number of connections is specified on curl easy and multi handles, and not curl share handles. CURLOPT_MAXCONNECTS is a CURLOPT constant, meaning it's used in curl_setopt, and CURLMOPT_MAXCONNECTS is a CURLMOPT constant, meaning it's used in curl_multi_setopt; both are not usable with curl_share_setopt.

[ericnorris]

I've created the RFC at https://wiki.php.net/rfc/curl_share_persistence_improvement. I'll begin working on the current suggested PR changes shortly.

[TimWolla]

I've created the RFC at https://wiki.php.net/rfc/curl_share_persistence_improvement.

Thank you. I've had a read through it. The only remark I have is clarifying whether or not those handles can explicitly be closed to “reset” them. I can imagine both widening curl_share_close to also accept a persistent handle and adding a new curl_share_close_persistent function - and also not providing this option.

Other than that it's looking good to me.

[ericnorris]

The only remark I have is clarifying whether or not those handles can explicitly be closed to “reset” them. I can imagine both widening curl_share_close to also accept a persistent handle and adding a new curl_share_close_persistent function - and also not providing this option.

Interesting. By "reset", do you mean that calling curl_share_close would destroy and re-initialize the persistent handle, as opposed to just closing it?

I think I would be against allowing a user to close a persistent share handle, since it would leave the object in an invalid state (the previous PR currently does allow this, however). Resetting would probably be fine, but it's not immediately clear to me when that would be useful.

I'd probably lean towards not providing the option for now, so I might add that under 'Rejected Features' and note that we could add support for this in the future?

[TimWolla]

Some nits. Implementation overall LGTM. Thanks for the RFC.

[Girgias]

Some comments, also the tests could try/catch so you don't print the whole stack trace.

[ericnorris]

Thanks @TimWolla! Is there anything else needed from me before this can be merged? I'm up for helping with the documentation as well, but that process would be entirely new to me.

[nielsdos]

Looks good

[nielsdos]

Is there anything else needed from me before this can be merged?

I don't think so. We might wait a bit on the code owner of the curl ext to see if he has comments, but he's not actively maintaining the curl ext right now so maybe he won't comment.

I'm up for helping with the documentation as well, but that process would be entirely new to me.

The documentation lives in a separate repo. See https://github.com/php/doc-en

[ericnorris]

I don't think so. We might wait a bit on the code owner of the curl ext to see if he has comments...

Makes sense.

The documentation lives in a separate repo. See https://github.com/php/doc-en

Got it, I'll start a PR there and I assume we'll wait to merge that until this is merged.

[TimWolla]

Just added UPGRADING/NEWS and merged the latest master. Will wait for the CI and then merge, any further changes can then be made as a follow-up. Thank you 😃

[TimWolla]

Now merged. Don't forget to move the RFC to the “Implemented” section and update it with the resulting implementation commit and status.

[ericnorris]

Thanks @TimWolla and everyone else for the feedback! I'm happy with the result, and I hope others are too. I'll follow up with a documentation PR shortly.