Implement Fibers

[trowski]

Implementation of the Fibers RFC.

[nikic]

<?php
new class() {
    function __destruct() {
        $fiber = new Fiber(static function() {
            Fiber::suspend();
        });
        $fiber->start();
        throw new Exception;
    }
};

Zend/zend_exceptions.c:994: void zend_throw_fiber_exit(void): Assertion `!(executor_globals.exception)' failed.

[nikic]

Hm, this trace is not super clear to me. Does {main} here refer to the top of the fiber, and the caller of resume() is not present at all?

I feel like at the very least there should be an indication that we're inside a fiber here.

[trowski]

Is there a way we could change [internal function] to something like [fiber] instead?

[trowski]

One suggestion I have is swapping {main} to {fiber} in the exception backtrace: trowski@8265319

I attempted to save a pointer the call stack invoking start, but a fiber is able to long outlast the stack that may have started it. It's also undesirable to swap the stack trace on each resume, as this will drop the backtrace context in the fiber, which leads to the terrible backtraces we often had with Generators in Amp.

[nikic]

One suggestion I have is swapping {main} to {fiber} in the exception backtrace: trowski/php-src@8265319

Adding a bool for this seems really ad-hoc ... would it be valuable for something if the exception contains a reference to the containing Fiber object outright?

I attempted to save a pointer the call stack invoking start, but a fiber is able to long outlast the stack that may have started it. It's also undesirable to swap the stack trace on each resume, as this will drop the backtrace context in the fiber, which leads to the terrible backtraces we often had with Generators in Amp.

Not sure I follow. To be clear, what I would expect is the backtrace at the point of resume, not at point of start. For generators, this is implemented with a dummy frame that gets resolved when the backtrace is actually fetched. Why are the resulting backtraces "terrible"?

[kelunik]

@nikic Including resume() doesn't really make sense, because you want the complete fiber stack there, where resume() would be somewhere in between. But having {main} there is really weird, I guess it should be {fiber}.

[nikic]

Do we need these methods on ReflectionFiber if they're already available on Fiber?

[trowski]

The RFC did include them, but you can just as easily do $reflection->getFiber()->isSuspended(). I'm fine with removing them, we can always add them back.

[kelunik]

Should we instead skip ReflectionFiber entirely and add the methods to Fiber?

[nikic]

That might make sense. The current split is basically just "these are only used for debugging"?

[kelunik]

Yes, basically.

[trowski]

<?php
new class() {
    function __destruct() {
        $fiber = new Fiber(static function() {
            Fiber::suspend();
        });
        $fiber->start();
        throw new Exception;
    }
};

Zend/zend_exceptions.c:994: void zend_throw_fiber_exit(void): Assertion `!(executor_globals.exception)' failed.

Fixed in f062904.

[nikic]

The observer functionality should be tested via zend_test.

[trowski]

The observer functionality should be tested via zend_test.

I moved the observer functionality to zend_observer and added tests in zend_test.

[trowski]

This issue still exists, just in different variations, e.g. this one causes use-after-free

Can you double check this is fixed now? I'm not seeing it anymore, but I want to be sure you're not either.

[nikic]

This issue still exists, just in different variations, e.g. this one causes use-after-free

Can you double check this is fixed now? I'm not seeing it anymore, but I want to be sure you're not either.

Yeah, this works fine now.

[nikic]

This will need a small rebase due to some conflicting error handling changes I've made.

[trowski]

This is using boost context 1.76. However, I'm only bundling the files that can be resolved by configure. I added the s390x files, looks to work fine.

[bwoebi]

While reviewing the observers handling, I've noticed that first_observed_frame and current_observed_frame are unfortunately global, as well as zend_observer_fcall_end_all only affecting a single fiber, instead of them all.

In PHP 8.0 there's a guarantee that all begun observer callback also will have the end callback called. With fibers this seems to not be true in a bailout case at least.

[nikic]

Can we kill zend_observer_fcall_end_all() and let the extension deal with it?

[bwoebi]

I'm pretty unsure about this. I guess an extension can keep track of all currently begun observers, prepend a custom internal callback to BG(user_shutdown_function_names) and then cleanup itself. This most probably is the least error prone way. This first_observed_frame and current_observed_frame seems really susceptible for being invalid...
But obviously only an option for PHP 8.1. Let me also ask @morrisonlevi whether he has some input here.

[morrisonlevi]

Can we kill zend_observer_fcall_end_all() and let the extension deal with it?

Not sure I understand. It is important to the design of observers that each extension doesn't have to handle weird cases where they have to end things manually. Are you asking to move this out of the engine and into every extension? If so, I must object. These edge cases change over time, as the new fibers case show. We didn't even make it one minor version without a new edge case!

[nikic]

I would expect this to be a lot easier to handle for an extension, because it knows which open internal state it has that needs to be closed. It doesn't need to integrate with any of the actual observer infrastructure.

I grudgingly accepted this hack back then on the premise that it at least won't hurt, but that clearly hasn't held up. If there is no robust implementation of this functionality (and no, @bwoebi's recent commit just adds one more hack on top of the hack) then I'm not willing to provide it at all.

[SammyK]

If there is no robust implementation of this functionality

Zooming out, the only robust solution I see is to sunset zend_bailout across the board.

[bwoebi]

Yep, my fix is just for an actual issue people are facing, I'm definitely not happy with this as a long term solution.
On the topic of moving this to the extension, it should be pretty easy at least for my extension to work around this.

@SammyK I really don't know how doable sunsetting zend_bailout is ... I mean, any OOM will trigger a bailout for example, but shutdown sequence shall still happen.

[SammyK]

OOM will trigger a bailout

What is blocking OOM from doing a clean shutdown similar to unwind exit?

[morrisonlevi]

For PHP 8.1 I think we should try to play whack-a-mole with the issues and keep zend_observer_fcall_end_all. For v8.2, we should make a list of all zend_bailout calls and decide whether 1) we consider that a legitimate usage or if it should be migrated to a different error handling mechanism and 2) if the ones that remain are sufficiently edge-case that we can accept not calling observers' .end for these. What does everyone think about that?

[SammyK]

For v8.2, we should make a list of all zend_bailout calls and decide

I like this. FWIW I started working on a big "sunsetting zend_bailout" doc earlier this year but lost steam on it. Maybe when winter hits I'll get bored and start working on it again. 😄

[bwoebi]

What is blocking OOM from doing a clean shutdown similar to unwind exit?

OOMs can happen in midst of any function basically, and all our code assumes that emalloc() will return a valid chunk of memory. So basically everything would horribly break if we were to return an invalid pointer (incl. NULL).

[SammyK]

all our code assumes that emalloc() will return a valid chunk of memory

Bah. Good point. We can revisit this issue while trying to find a path to sunset zend_bailout a bit later.