On Fri, Nov 1, 2024 at 2:59 PM Gina P. Banyard <intern...@gpb.moe> wrote: > > On Wednesday, 9 October 2024 at 21:16, Eric Norris <eric.t.nor...@gmail.com> > wrote: > > > Hello all, > > > > After receiving some feedback about > > https://github.com/php/php-src/pull/15603, I'm formally proposing an > > RFC to add persistent curl share handles here: > > https://wiki.php.net/rfc/curl_share_persistence > > > > Thanks to those who have provided feedback so far! Of note: the > > implementation introduces a global variable to the curl extension via > > ZEND_BEGIN_MODULE_GLOBALS; this appears preferred over > > EG(persistent_list). Should the implementation stick with the module > > global, or should it go back to EG(persistent_list) until we've > > created a formal non-resource API (in another RFC)? > > > > I'd like to keep the future of persistent resources out of scope for > > the decision on whether to add curl share handle persistence to PHP, > > but it is worth deciding what form the persistence takes for now. > > I have voted against the RFC as it lacks details and there seems to have been > no analysis on what sort of issues this can cause.
I appreciate your feedback, but from my perspective there aren't really any major issues that this can cause. We already support curl share handles via curl_share_init, and adding persistence doesn't fundamentally change the issues you may have with reusing a curl handle. Tim pointed out that libraries may use this functionality - and I hope they do, as it can improve performance - in a way that is dangerous for users, but I frankly don't see that as likely. HTTP request libraries will likely be interested in the connection sharing aspect of this, not the cookie jar. If they were to enable cookie sharing, they would need to do that in a way that was safe for users, much like they would for any other setting in PHP. It's worth noting that setting CURLOPT_COOKIEJAR is already possible with PHP today. > This has been pointed out by Tim already, and this should have been caught > earlier, but I don't know if it just me, but it seems your email kinda went > into some void and got unnoticed. Thank you for taking the time to respond at all, late or not. > Persistent handlers don't really fit PHP's model, and are practically > untestable from a PHPT test perspective as far as I know (at least I have > never seen any proper tests for the existing ones, but I am happy to be > corrected). Persistence is important to successfully scaling Etsy's monolithic PHP application. As I mentioned in a response to Tim, we make use of persistence for things like memcached connections in order to reduce end-user latency, and we'd like to take that further by persisting the connections of downstream curl requests we make. This is not just an Etsy-specific issue; https://blog.jetbrains.com/phpstorm/2024/10/php-annotated-october-2024/#%F0%9F%93%8A-rfc-add-persistent-curl-share-handles notes that other large-scale PHP users are interested in persistent HTTP(S) connections. I'd implore you to consider that this would have a real-world impact on companies that make use of ext/curl in PHP, without needing to use alternative runtimes or external software. > As such I am very reluctant to adding more of these things to the language, > especially when this seems to just have been hacked together quickly, we > still have at least 7 months before feature freeze and I would prefer to > actually be done properly. I'd like to note that I don't feel like this was "hacked together quickly". I take pride in writing high-quality code, and I've attempted to request feedback multiple times in both the PR and in the internals mailing list. I plan on implementing tests as well as I possibly can assuming the RFC passes, but I did not want to invest in doing so unless it did pass. If there are any quality issues you see in the PR, please feel free to point them out! @kocksismate already noted a few changes that I planned on making. > Especially that it is always easier to add new features than to deprecate and > remove bad ones (which is kinda backwards but not relevant to this > discussion). If we truly feel that persistence is wrong for PHP, we would need to deprecate and remove things like persistent PDO connections; I would expect that would be a perfectly fine time to also remove support for persistent curl share handles, if they existed. Thanks, Eric
