Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125891
Precedence: bulk
MIME-Version: 1.0
References: <CA+NMNO2Zizv7mrzmxxiJ8t8yYVLGb-=KgwyvWUffo3ohdaOW-A@mail.gmail.com>
 <fHnLD3yEum71TAgA9Lf0jamB5V3zPTu0W0pRBWlXMl04DAeHZ7vIvNQTBWhEkZAQ_cg8JJkkalFBTJWv_CMaO7qBVtRZ9e9e0F2KMUfbsbw=@gpb.moe>
In-Reply-To: <fHnLD3yEum71TAgA9Lf0jamB5V3zPTu0W0pRBWlXMl04DAeHZ7vIvNQTBWhEkZAQ_cg8JJkkalFBTJWv_CMaO7qBVtRZ9e9e0F2KMUfbsbw=@gpb.moe>
Reply-To: erictnorris@gmail.com
Date: Fri, 1 Nov 2024 16:07:39 -0400
Message-ID: <CA+NMNO2rv+Dy_Bw9ovrd6o=OpOijFt+FWfuV-6Fr0BXVwwKOVQ@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Persistent CurlShareHandle objects
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: eric.t.norris@gmail.com (Eric Norris)

On Fri, Nov 1, 2024 at 2:59=E2=80=AFPM Gina P. Banyard <internals@gpb.moe> =
wrote:
>
> On Wednesday, 9 October 2024 at 21:16, Eric Norris <eric.t.norris@gmail.c=
om> 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 caugh=
t earlier, but I don't know if it just me, but it seems your email kinda we=
nt 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 unt=
estable from a PHPT test perspective as far as I know (at least I have neve=
r seen any proper tests for the existing ones, but I am happy to be correct=
ed).

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 languag=
e, 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 a=
ctually 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 dis=
cussion).

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