Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:113555
MIME-Version: 1.0
References: <D376DEFE-26A4-4F11-8548-D9454087E9FD@trowski.com>
 <70951423-5e77-c150-6dce-dd3c62f8dc8b@php.net> <0b994a60-7970-605b-1657-d6ee732690e5@gmx.de>
 <5C73A1DE-E563-4F69-B8C7-6506F81D7345@trowski.com>
In-Reply-To: <5C73A1DE-E563-4F69-B8C7-6506F81D7345@trowski.com>
Date: Tue, 16 Mar 2021 09:30:36 +0000
Message-ID: <CAKiSzdDVR+joZQQtAS03rAZE_HBZir1CkaBG-BJm6Da1xLnu6A@mail.gmail.com>
To: Aaron Piotrowski <aaron@trowski.com>
Cc: "Christoph M. Becker" <cmbecker69@gmx.de>, Michael Wallner <mike@php.net>, 
	php internals <internals@lists.php.net>, Derick Rethans <derick@php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [VOTE] Fibers
From: joshdifabio@gmail.com (Josh Di Fabio)

On Fri, Mar 12, 2021 at 10:54 PM Aaron Piotrowski <aaron@trowski.com> wrote=
:
>
>
> > On Mar 12, 2021, at 4:36 PM, Christoph M. Becker <cmbecker69@gmx.de> wr=
ote:
> >
> > On 12.03.2021 at 23:04, Michael Wallner wrote:
> >
> >> Thank you, and everyone involved, for your effort.
> >>
> >> On 08/03/2021 20.40, Aaron Piotrowski wrote:
> >>> Greetings everyone!
> >>>
> >>> The vote has started on the fiber RFC: https://wiki.php.net/rfc/fiber=
s <https://wiki.php.net/rfc/fibers>
> >>>
> >>> Voting will run through March 22nd.
> >>
> >> I voted /no/, because of the dependency on Boost.
> >> If my assumptions are wrong, I may reconsider my vote.
> >
> > Only asm files are used[1], and these can be bundled, so there is no
> > dependency on boost or C++ in general.
> >
> > [1] <https://github.com/amphp/ext-fiber/tree/master/boost>
> >
> > --
> > Christoph M. Becker
> >
>
> Hi Mike, Christoph, and Derick,
>
> To add a bit more information:
>
> These asm files are part of the low-level boost.context lib, found here: =
https://github.com/boostorg/context
>
> This library has infrequent releases. Some of the files for old architect=
ures have not changed in several years. Keeping these files up-to-date will=
 not be a burden (and I plan to assume this responsibility).
>
> The Boost license is extremely permissive and approved by the OSI, so the=
re is no problem bundling with PHP.
>
> Hopefully that provides some clarification and you=E2=80=99ll reconsider =
your vote Mike.
>
> Cheers!
> Aaron Piotrowski
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>

Apologies, this is a long one!

This RFC strikes me as being very dangerous. Implicitly allowing code
which is synchronous by design to be executed asynchronously seems
sure to lead to very subtle, unpredictable, confusing and dangerous
bugs.

The issue is that executing code asynchronously introduces race
conditions and, in code which is synchronous by design, those race
conditions will lead to bugs which will never occur during QA or
development and will be incredibly hard to identify once they do occur
in production.

These are the kind of bugs where user sessions get mixed up, the wrong
package is installed by a package manager, or an ecommerce website
captures the wrong amount of money from a customer's credit card. And
developers will have no realistic way of tracking down why these
things are happening in real world, multi developer, multi vendor PHP
applications.

For example, the PHP ecosystem is full of code like this:

    private function capturePayment()
    {
        $paymentRequest =3D preparePaymentRequest($this->currentOrder);
        $this->paymentGateway->capturePayment($paymentRequest);
        $this->currentOrder->setTransactionId($paymentRequest->getTransacti=
onId());
    }

In the above code snippet, the developer has relied on the fact that
the mutable value $this->currentOrder will be the same before and
after the call to $this->capturePayment(), because capturePayment() is
synchronous. In all likelihood, the authoring PHP developer is not
even familiar with asynchronicity, and the assumption that
capturePayment() is synchronous is therefore not even a conscious one.
Note that, if capturePayment is executed asynchronously, eventually we
will see payments attributed to the wrong orders.

Note that, if fibers land, there is no know whether the code above is
correct by looking at it in isolation. You can only know that by
knowing the environment in which the code is executed, as well as
understanding *all of the internals of capturePayment()*, and its
dependencies, many of which will probably managed by other developers
and will involve a bunch of injected services, many of which will do
IO and therefore are candidates for using fibers, directly or
indirectly, such as logging, HTTP, and database.

The worst characteristics to me are the following:
- These race conditions will cause problems so rarely that developers
will not even realise that their programs contain dangerous bugs. Code
which is not safe to use with fibers will appear to be functional
because the happy path will work fine, and so developers will assume
it's okay.
- Changes very, very far away from your code can cause your code to
become unexpectedly asynchronous. Imagine you are using the DynamoDB
monolog logger in your application, and that library indirectly uses
Guzzle, which gets fiber support. Any place you are logging anything
now becomes asynchronous, as well as any code which invokes that code.
If you are using some kind of event dispatch pattern then pretty much
everything in your program becomes asynchronous.
- As a library author, there is no way of knowing how your code is
going to be used, and what injected dependencies might use fibers
internally.

The counter arguments:
- "The above code snippet is suboptimal, it shouldn't assume that
mutable state remains unchanged following some IO, and the order
should be passed in as an arg." -- Whether or not this is true is
irrelevant -- developers have relied on the invariants of the PHP
language when writing their code, such as synchronous code being
synchronous, and this is how lots of code is written in the wild. If
this feature lands, that won't change -- very few userland developers
will understand this feature, including library authors.
- "Fibers should only be enabled in async first programs." and "We
could flag that certain libraries are fiber compatible or
incompatible." If code appears to work in a fiber environment, people
will just use it in a fiber environment, without understanding all of
the security issues and horrible bugs they're introducing. This
problem should rather be solved at the language level by making async
explicit.

Consider the following:

    private async function capturePayment()
    {
        $paymentRequest =3D preparePaymentRequest($this->currentOrder);
        await $this->paymentGateway->capturePayment($paymentRequest);
        $this->currentOrder->setTransactionId($paymentRequest->getTransacti=
onId());
    }

Looking at the above code, I can see the race condition. It becomes
explicit. So I change the code and communicate explicitly via the type
system that this function is asynchronous and that my code expects
PaymentGateway::capturePayment() to be asynchronous.

    private async function capturePayment()
    {
        $order =3D $this->currentOrder;
        $paymentRequest =3D preparePaymentRequest($order);
        await $this->paymentGateway->capturePayment($paymentRequest);
        $order->setTransactionId($paymentRequest->getTransactionId());
    }

Regarding PHP's type system not supporting generics, and the
associated advantages of this RFC over promises; I agree that this is
a problem with promises. But the lack of generics in PHP already means
we have no language-level type safety on data structures, which is
certainly worse than not having type safety on promises. Psalm et al
have provided reasonable solutions to this issue.

Sorry for the criticism of the idea -- I've used React and Amp in
production on a product which has been in prod for many years -- and
I'd be happy to see first class asynchronous support within the
language, but I think this RFC would make the language worse, not
better. Technically, I'm sure this is great, and I do appreciate that
a lot of work has probably been put into this.

Thanks