One Loop to Rule Them All

[davidwdan]

This PR is to discuss an alternative to the issues raised in #139

By getting rid of the ability to stack loops and only having a single loop, we can simplify Loop and allow projects to use it in which ever way they're accustomed to. I think that the event loop spec should be less opinionated.

Use Case 1 (Amp style)

Loop::execute(function () {
    //Do all async inside of an execute

    Loop::defer(function () {
        echo '1' . PHP_EOL;
    });
});

Use Case 2 (React Style)

Loop::defer(function () {
    echo '2' . PHP_EOL;
});

Loop::run();

Use Case 3 (Mix and Match)

Loop::defer(function () {
    echo '3' . PHP_EOL;
});

Loop::execute(function () {
    Loop::defer(function () {
        echo '4' . PHP_EOL;
    });
});

Loop::defer(function () {
    echo '5' . PHP_EOL;
});

Loop::run();

For tests, we can provide a reset function as part of event-loop-test, which would allow you to reset the Loop state between tests.

function reset()
{
    $ref     = new \ReflectionClass(Loop::class);

    $driver  = $ref->getProperty('driver');
    $driver->setAccessible(true);
    $driver->setValue(null);
    $driver->setAccessible(false);

    $factory->setAccessible(true);
    $factory = $ref->getProperty('factory');
    $factory->setValue(null);
    $factory->setAccessible(false);
}

[kelunik]

For tests, we can provide a reset function as part of event-loop-test, which would allow you to reset the Loop state between tests.

Making it part of event-loop-test doesn't really make sense, as that's a package to be used by loop implementations to check their implementations, not by libraries. If we choose to have non-stacked loop, I think we should have a proper reset method on the accessor instead of using reflection hacks. Tests are a very much needed use case that shouldn't require reflection hacks.

Regarding use case 3, what does Loop::execute do there? Just the same as Loop::defer($cb); + Loop::run();?

[davidwdan]

I'm not particularly attached to reset with reflection. We just need to provide a way to reset.

Case 3 is just to show that you can use the different methods together. With the current interop loop, the following code will only echo "4";

Loop::defer(function () {
    echo '3' . PHP_EOL;
});

Loop::execute(function () {
    Loop::defer(function () {
        echo '4' . PHP_EOL;
    });
});

Loop::defer(function () {
    echo '5' . PHP_EOL;
});

[kelunik]

@davidwdan If we go that way, having multiple ways to do the same thing doesn't make sense to me.

[WyriHaximus]

@davidwdan agreeing with @kelunik here, duplication should be avoided here. Have you considered:

Use Case 1 (Amp style)

Loop::defer(function () {
    echo '1' . PHP_EOL;
});

Loop::execute(function () {
    // Do all async inside of an execute,
    // however everything registered with the loop before Loop::execute
    // is also executed.

    Loop::defer(function () {
        echo '2' . PHP_EOL;
    });
});

Use Case 2 (React Style)

Loop::defer(function () {
    echo '1' . PHP_EOL;

    Loop::defer(function () {
        echo '2' . PHP_EOL;
    });
});

Loop::execute(); // The closure is optional

Both output:

1
2

This way both API's and accustomed ways of working will be preserved (unless I'm overlooking something).

[kelunik]

I'm not interested in keeping the current state, I don't care. I want a solid interoperable specification instead of accounting for current behavior where code has to be rewritten anyway. So the current use doesn't really matter to me. I think we should specify how we want it, because I think the specification will have a long future, instead of looking back and taking historical "cruft" with us.

The main reason why React does it the current way is because there's no other possibility. You need to create the loop and pass it everywhere. Well, there could be something like ...

React\run(function ($loop) {
    // set up here and run afterwards
});

... but with loops being created manually, I think the current way makes sense for React. The question is whether it still makes sense if there's a accessor now, or whether scoping is something we want to clearly separate sync and async code.

[mbonneau]

@kelunik - The React pattern of using event loops is standard across many languages/event libraries (boost asio in c++, Twisted in python). I also know that just because lots of other people do it that way does not necessarily mean it is the correct way for this group. But it is well understood and used by many. I would not consider it "cruft" - just a different way of doing things.

I think we should specify how we want it

It is great to aim for a library that is done exactly right. The problem is that different people will have different ideas of right. Interop needs to meet on a middle ground to fulfill the "interop" in its name.

@WyriHaximus's suggestion (along with this PR) would be a good compromise.

[kelunik]

The React pattern of using event loops is standard across many languages/event libraries (boost asio in c++, Twisted in python).

I'm not familiar with either of them, but I just looked up how testing with Twisted works. They have their own test runner which runs the loop and requires tests to clean up everything, otherwise the test will error. I don't know whether they reset the reactor afterwards internally or whether they just stop after the first error where the loop isn't cleaned up after a test.

Twisted also doesn't allow to run a reactor multiple times, which is required to have multiple wait()s for mixing sync and async code.

Testability is a major concern and I think it's particularly important to be able to start each unit test with a clean loop.

But it is well understood and used by many. I would not consider it "cruft" - just a different way of doing things.

That's exactly why I put it in quotes, I just didn't find a better word for "it's the current way of doing things, but not necessarily the best way we want for the future."

It is great to aim for a library that is done exactly right. The problem is that different people will have different ideas of right. Interop needs to meet on a middle ground to fulfill the "interop" in its name.

I know and I'm fine with that as long as "meet on a middle ground" isn't "having both". Having a single way of doing things is more important to me than having a specific way.

[bwoebi]

I have one major concern with this.

Tests. Well you provide a solution, but it's ugly.

And, even with an explicit reset method; if you forget to reset once, many tests may fail subtly. That was the primary point of the scoping.

Apart from that, continuing a loop in a shutdown handler must work. (for cleaning up etc.) Or using a new, separate, clean loop there. (e.g. Aerys doesn't want to be stuck in a startup loop, so it needs to signal that it was a startup issue. … Inside a clean loop which we can be sure won't fatal again.)

[kelunik]

Let me summarize the requirements we have:

• Run a loop
• Continue to run that same loop in a shutdown function
• Run a new loop in a shutdown function to ensure everything is clean (but another shutdown hook might want to continue running the previous loop)
• Having a clean loop per unit test

Swapping works for point three, but stacked loops simplify the case where a second shutdown handler exists expecting the previous loop to continue.

[bwoebi]

@davidwdan Further input from you would be appreciated, if you want to continue the conversation.

[davidwdan]

I'm not sure that I have much more input. Aside from testing (which can be solved with reflection) the other use cases for stacked loops seem like edge cases. I would much rather "hack" testing than force one way of doing things onto everyone, at the risk of alienating other async projects.

@WyriHaximus's solution seems like a good compromise.

[bwoebi]

These are edge cases, but there shall be a way to support them. I'm not opposed to not making these particularly simple, but there must be some way. Doesn't matter much how I can do it, as long as I don't have to resort to reflection for that.

I'm fine with @WyriHaximus solution, if I can still replace the event-loop.

[kelunik]

Aside from testing (which can be solved with reflection) the other use cases for stacked loops seem like edge cases.

It's 100% right that stacked loops are edge cases. It's also right that testing could be solved with reflection, even if I prefer an explicit reset method then. The real point of Loop::execute is not allowing stacked loops, the real point is having a very clear scope.

Resetting the loop might be forgotten, but your tests might still work, because the test clears everything up or the additional still registered events do not change anything. But there might be a future change that changes that, breaking your tests. Now you have no idea it's caused by a still registered event from a totally unrelated test. It's something that just can't happen with the clear Loop::execute scoping.

I would much rather "hack" testing than force one way of doing things onto everyone, at the risk of alienating other async projects.

This specification does not only aim for pure interoperability of all currently existing loop implementations. The goal includes defining how event loops will work in the future. It's the exact reason why the specification also includes the Loop accessor additionally to the Driver interface.

While React, Amp, and Icicle are all pretty stable, async PHP is still rather young and newish. If 100% BC compatibility and a common loop implementation were the goal, we could just kill Amp and Icicle and other existing event loop implementations (because lower usage) and use React, but the reason alternative event loop implementations exist is that people thought React's way of doing things can be improved. That's why we're here now, to find the best way for the long-term future.

Or to quote @rdlowrey:

The point is to figure out, "what is the right way to go forward," not "how do we preserve BC for everyone"

FWIW we passed the loop around just like React does now. We changed that for Amp v1.0.0 and have since then gained experience with the global accessor. With these 1.5 years of experience, @rdlowrey, @bwoebi, and I all agree that the Loop::execute way is the best way to move forward.

It's not about alienating existing projects. Whichever way we choose, we'll have code that need updates to use the new loop specification instead of their current loop. And again, it's only bootstrapping code that's affected by a clear scope, so it should only affect application bootstrapping and tests, all existing libraries should work fine with the adapter until they're upgraded.

[davidwdan]

@kelunik I think you're missing the point. This project is starting to feel more like Amp Loop v2 than an interop project and your entire previous comment just reenforces that sentiment.

async PHP is still rather young and newish

Perhaps too new to be making a standard...

This discussion has run its course, so I'm going to close this PR.

[bwoebi]

@davidwdan Well, Amp v2 was born out of this. The interop was the main reason why we started Amp v2.

All we are trying is bringing forward the lessons we learned with Amp. In the alphas of Amp v1 (and 0.x versions) we had the loop as injected dependency, just like react, with a ->run() at the end. In Amp v1 we moved to a global loop, which considerably increased our pleasure working with async. After some time we began realizing that there were a few things lacking, like the scoping.

Though… If you think that's not a good idea, may I propose something else? Just adding (in addition to the current methods):

function run() {
    self::get()->run(); // (The actual code will be a bit more complex, but that's just the gist)
}

That's it. We still have the benefit of scoping where we need it (e.g. tests) and we can easily use the react style where we want to:

Loop::defer(function () {
    echo '1' . PHP_EOL;

    Loop::defer(function () {
        echo '2' . PHP_EOL;
    });
});

Loop::run();

Does that sound like an acceptable compromise?

\cc @WyriHaximus @mbonneau

[WyriHaximus]

Does that sound like an acceptable compromise?

To me on this point it does. But after a chat with @clue, @jsor, and @cboden we've come to the conclusion ReactPHP won't adept to the AI loop in its current form. We feel this project has lost it's focus on interoperability between loop projects, and that the resulting spec adds more complexity then needed for something that was supposed to be a global accessor and an interface. We'd love to have true interoperability between projects working together at a simple interface.

[bwoebi]

@WyriHaximus The big issue if we just share the interface is that we end up not being interoperable at all.

Then Amp has its global accessor and React does. Changes on the one accessor then wouldn't reflect on the other.

What we could do instead is dropping the Loop class as is (retaining the Driver interface) and replacing it by a trivial get/set; and libraries would then use their specific accessor API.

Would that work? — I opened #149, please answer there.

[Thus it would be the task of the accessor API to scope or implement whatever restrictions it wants.] [We then can introduce the complex Loop accessor at a later point where we all agree on that or not at all.]

[kelunik]

Closing, as project discontinued for now.