Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:113572
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> <CAKiSzdDVR+joZQQtAS03rAZE_HBZir1CkaBG-BJm6Da1xLnu6A@mail.gmail.com>
 <CANUQDCg-TQCC6LBhDBRvXse2CpVODmM5cK5==a-yMOS09VYeGg@mail.gmail.com>
In-Reply-To: <CANUQDCg-TQCC6LBhDBRvXse2CpVODmM5cK5==a-yMOS09VYeGg@mail.gmail.com>
Date: Tue, 16 Mar 2021 18:44:12 +0000
Message-ID: <CAKiSzdA+6YK69ez_9kZGACTNMi_e44xX_UPLfDCoU1i0qrF-ew@mail.gmail.com>
To: Niklas Keller <me@kelunik.com>
Cc: Aaron Piotrowski <aaron@trowski.com>, "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 Tue, Mar 16, 2021 at 6:20 PM Niklas Keller <me@kelunik.com> wrote:
>
> Hey Josh,
>
>>
>> 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.
>
>
> Thank you for sharing your thoughts, I'll respond inline for better conte=
xt, but shortened some paragraphs for better readability.

Many thanks for the detailed reply. I've replied inline.

>>
>> [...]
>>
>> 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->getTransa=
ctionId());
>>     }
>>
>> 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() [...]
>
>
> This is a very valid concern to have. However, this code won't simply bre=
ak if executed asynchronously.
> It only breaks if the same method (or other methods making use of the sam=
e state) is executed concurrently on that object.

I understand this, but of course this will be common in programs where
fibers are used, and the nature of this proposal means that fibers
will potentially be highly insidious and quite unpredictable in their
reach.

>>
>> [...]
>>
>> - These race conditions will cause problems so rarely that developers
>> will not even realise that their programs contain dangerous bugs. [...]
>> - Changes very, very far away from your code can cause your code to
>> become unexpectedly 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." [...]
>> - "Fibers should only be enabled in async first programs." and "We
>> could flag that certain libraries are fiber compatible or
>> incompatible." [...]
>>
>> Consider the following:
>>
>>     private async function capturePayment()
>>     {
>>         $paymentRequest =3D preparePaymentRequest($this->currentOrder);
>>         await $this->paymentGateway->capturePayment($paymentRequest);
>>         $this->currentOrder->setTransactionId($paymentRequest->getTransa=
ctionId());
>>     }
>>
>> 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.
>
>
> If you can see the race condition here, you can probably also see the rac=
e condition in the original snippet above.

The difference is that in the second case, the developer has
*explicitly opted into the underlying call being async*. I.e. they
expect it, and can design with it in mind.

> The overall issue you're describing is thread safety and mutable state. I=
t exists with fibers, but a very similar issue exists with explicit async /=
 await.
> Just because something supports asynchronous I/O, doesn't mean it also su=
pports concurrent operations on its state.
>
> It might be a good idea to introduce some guarding concept in a follow-up=
 RFC, so objects can easily protect against being used concurrently e.g.
>
> ```
> private synchronized function capturePayment()
> {
>      $paymentRequest =3D preparePaymentRequest($this->currentOrder);
>      $this->paymentGateway->capturePayment($paymentRequest);
>      $this->currentOrder->setTransactionId($paymentRequest->getTransactio=
nId());
>  }
> ```
>
> "synchonized" isn't the right keyword here, but I can't think of a better=
 name right now.
>
>>
>> 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.
>
>
> Being able to use the type system is a concern the RFC solves, but that's=
 rather just nice-to-have. The relevant problem is the call stack pollution=
 or "What color is your function" problem. It simply doesn't make sense for=
 applications and libraries to offer an async and non-async interface for e=
verything. Due to the "What color is your function" problem, there can't be=
 a gradual migration to non-blocking I/O, which will mostly result in non-b=
locking I/O simply not being supported by the majority of libraries, making=
 it basically non-viable in PHP.

Perhaps we could rather make fibers *opt in* at the *callsite*
(similar to goroutine calls) in order to prevent functions
unexpectedly being executed asynchronously due to faraway changes.
This would be safe and predictable while also avoiding the "What color
is your function" problem. For example

    private function capturePayment(): void
    {
        $paymentRequest =3D preparePaymentRequest($this->currentOrder);
        // allow, but don't require, the underlying call to use
fibers. Callers higher up the stack would also have to opt in
        async $this->paymentGateway->capturePayment($paymentRequest);
        $this->currentOrder->setTransactionId($paymentRequest->getTransacti=
onId());
    }

With this approach, APIs we'd avoid the "what colour is your function"
problem without sacrificing the safety that we really need in e.g.
ecommerce.

>
> Best,
> Niklas

Thanks again for engaging.