Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve fiber interoperability #7128

Merged
merged 2 commits into from Jun 11, 2021
Merged

Improve fiber interoperability #7128

merged 2 commits into from Jun 11, 2021

Conversation

trowski
Copy link
Member

@trowski trowski commented Jun 9, 2021

Adds another global, EG(active_fiber) to track the instance of Fiber that is active, regardless of the currently running zend_fiber_context. This allows alternative implementations to not interfere with the operation of Fiber as well as allowing Fiber instances to contain such instances (and vice-versa), switching between fiber contexts as determined by each implementation.

$fiber = new Fiber(function () {
    $test = new TestFiber(function () {
       $value = Fiber::suspend(123);
       TestFiber::suspend($value);
    });
    var_dump($test->start()); // int(246)
});
$fiber->resume(2 * $fiber->start());

Consider a TestFiber that functions identically to Fiber (this is just for example purposes, a custom implementation would likely exist to schedule fibers differently than Fiber). The above example switches back to {main} when Fiber::suspend() is called within the TestFiber, as the Fiber instance was started from {main}, not within the fiber running for TestFiber.

A custom fiber implementation is then able to schedule fibers using their own mechanism without a call to Fiber::suspend() potentially breaking scheduling or invalidating the custom fiber context.

If in the future, PHP includes an internal scheduler (event-loop) to create and schedule fibers, this PR will allow Fiber to mix with such a scheduler. Custom user space schedulers of Fiber instances will continue to work, even if interrupted by the built-in scheduler.

$fiber = new Fiber(function () {
    // `async` creates a fiber from an expression scheduled by the internal scheduler.
    $future = async (function () {
        return Fiber::suspend(1); // Returning resolves the future.
    })();
    
    // `await` suspends the internal fiber until the future is resolved.
    var_dump(await $future); // int(2)
});

var_dump($fiber->start()); // int(1)
var_dump($fiber->resume(2)); // NULL

@trowski
Copy link
Member Author

trowski commented Jun 9, 2021

@kooldev Any feedback on this before I request another review?

I did attempt to implement this using a pointer to the previous context and following the list of previous pointers back to the fiber context, but it is possible for context switches to create a circle that no longer includes the context linked to the Fiber object, specifically ext/zend_test/tests/fiber_test_03.phpt.

@ramsey ramsey added the Feature label Jun 9, 2021
@ramsey ramsey added this to the PHP 8.1 milestone Jun 9, 2021
@kooldev
Copy link
Contributor

kooldev commented Jun 9, 2021

I think it is fine to have special handling for zend_fiber in this case as it will probably be the only fiber implementation that is not backed by some kind of scheduler. Mixing fibers that use different schedulers will be problematic or even impossible. I was finally able to integrate this with my playground async extension (I am using symmetric coroutines exclusively) and it complicates the logic quite a bit. This was my first test:

$fiber = new Fiber(function () {
    $test = async(function () {
        AsyncTask::yield(); // Yields back to $fiber
        // 123 is returned to $fiber->start(), multiplied by 2 and sent back as 246.
        // Afterwards 246 is the coroutine result and sent into await().
        return Fiber::suspend(123);
    });

    AsyncTask::yield(); // Starts $test coroutine
    var_dump($result = await($test)); // int(246)

    return $result / 2; // Becomes the return value of $fiber
});

$fiber->resume(2 * $fiber->start());

var_dump($fiber->getReturn()); // int(123)

This seems to be correct behavior but it is really hard to keep track of control flow...
Second test:

$defer = new Deferred();

$t = async(function (Awaitable $a) {
    $fiber = new Fiber(function () {
        return Fiber::suspend(123); // Sends 123 as return value of $fiber->start()
    });

    // Receives 123 from $fiber and multiplies it with 2.
    // Result is sent into $fiber and becomes the fiber return value.
    $fiber->resume($fiber->start() * await($a));

    return $fiber->getReturn(); // Return value is 246
}, [ $defer->getAwaitable() ]);

AsyncTask::yield(); // Starts coroutine $t
$defer->resolve(2); // Resolves awaitable $a with the value 2

var_dump(await($t)); // int(246)

@trowski Is this the kind of behavior you are expecting?

@trowski
Copy link
Member Author

trowski commented Jun 9, 2021

@kooldev Yes, this exactly the behavior I was expecting. Mixing different schedulers will probably be terrible due to the schedulers conflicting. It's like trying to use multiple event loops – one will probably block the other, but at least things won't crash. Note that above I was using the term "scheduler" somewhat broadly. A simple push/pull while loop sending and consuming values from a generator could be considered a scheduler. Perhaps a user land scheduler would do something unrelated to I/O and would not conflict with a built-in event loop. Such a scheduler would be similar to running a generator that suspends the current fiber (something we do in Amp v3).

I wanted to be sure a scheduler + async/await could coexist with Fiber and get the expected results, which I think you demonstrated is possible (and you seem to have implemented it fairly quickly too!). I'm sure this added some complexity to the implementation, but it shows Fiber can be compatible with a future built-in scheduler or an extension's scheduler. 👍

Do I anticipate anyone will actually use fibers within code designed for async/await like you did above? No, not really… the control flow is awful. 🤣

@trowski trowski requested a review from krakjoe June 9, 2021 23:39
@krakjoe krakjoe force-pushed the fiber-interop branch 2 times, most recently from 16f46f1 to 3071650 Compare June 10, 2021 13:41
@trowski trowski merged commit e6e6b3e into php:master Jun 11, 2021
@trowski trowski deleted the fiber-interop branch June 11, 2021 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants