Safe execute

[kelunik]

This ensures we have a safe execute context again and removes the side effect of setFactory. I think we could even allow setFactory when running now.

This has the slight disadvantage for sync applications that they have to wrap their setup code in Loop::execute, too.

[davidwdan]

@kelunik how would this work with this change?

[kelunik]

@davidwdan It would just call getPreviousDriver instead get / getDriver.

[davidwdan]

@kelunik with the change you suggested, running this code gives warnings and throws an error:

$loop = new \WyriHaximus\React\AsyncInteropLoop\AsyncInteropLoop();

$loop->addPeriodicTimer(1, function () {
    echo 'test';
});

$loop->run();

Output:

Warning:  assert(): The Loop accessor may only be used inside a Loop::execute scope. failed in /htdocs/reactphp-async-interop-loop/vendor/async-interop/event-loop/src/Loop.php on line 195

PHP Fatal error:  Uncaught Error: Call to a member function repeat() on null in /htdocs/reactphp-async-interop-loop/vendor/async-interop/event-loop/src/Loop.php:196

Run looks like:

    public function run()
    {
        Loop::execute(
            function() {},
            Loop::getPreviousDriver()
        );
    }

[kelunik]

@davidwdan That prooves it's a good change. You're running a loop manually, but something assumes you're using the accessor. This will accidentally result in two different loops and subtle bugs. This change prevents that form happening that easily.

[davidwdan]

That proofs it's a good change.

This change seems like a solution in search of a problem.

[trowski]

@davidwdan This change is trying to prevent people from creating watchers from outside the loop and then calling Loop::execute(), because those watchers will not be executed unless the result of Loop::get() is passed to execute(). This fails silently and is likely confusing for new users. This PR addresses this issue by asserting watchers are created within an active loop context, preventing these bugs.

Tests will need to be modified to accommodate this change, but it should have no affect on applications unless mistakes are made that will lead to bugs.

[kelunik]

@davidwdan Sorry, I got your code wrong. I thought it'd run an interop loop via ->run(), but that's the adapter that calls Loop::execute then.

The issue is the one @trowski already described. We had a clear execute scope at the every beginning when we decided to allow stacked loops to exist. We had to loosen that strict scope for wait, but I think the way proposed now is a way better way that still allows wait, as it restores that clear scope again and prevents the issues @trowski mentioned.

Another thing this change fixes is a default driver, which had to be reset as a side effect in Loop::setFactory. That side effect bugged me from the beginning, but I didn't see a better way at that time.

[kelunik]

@davidwdan The code you mentioned is only an issue for applications, not for libraries, right? As far as I know you don't have a accessor and rely on injection of the loop. So libraries only create watchers once the loop runs, not outside of Loop::execute? Applications setting up the loop would have to be adjusted, but they need an update to use the interop loop anyway.

[davidwdan]

@kelunik I've played around with this some more and it's not as bad as I thought it would be.

Using existing react apps before this change:

$loop = new \WyriHaximus\React\AsyncInteropLoop\AsyncInteropLoop();

$source = new React\Stream\Stream(fopen('omg.txt', 'r'), $loop);
$dest = new React\Stream\Stream(fopen('wtf.txt', 'w'), $loop);

$source->pipe($dest);

$loop->run();

After this update:

Loop::execute(function(){
    $loop = new \WyriHaximus\React\AsyncInteropLoop\AsyncInteropLoop();

    $source = new React\Stream\Stream(fopen('omg.txt', 'r'), $loop);
    $dest = new React\Stream\Stream(fopen('wtf.txt', 'w'), $loop);

    $source->pipe($dest);
});

This does change how react apps have worked in the past, which might hinder adoption.

[kelunik]

@davidwdan That's right, it affects the bootstrap code of React applications, but it's a single line to remove and two lines to add + some indenting. I think that change is totally worth for the benefit of a clear scope we get. I think we could even remove then Loop::setFactory restriction then, as it no longer has side effects.

This does change how react apps have worked in the past, which might hinder adoption.

I don't really think this hinders adoption. That kind of code is only used for bootstrapping in applications, no? It doesn't change anything for libraries and those are the ones we really care about being interoperable.

[joshdifabio]

What's the use case for getPreviousDriver()?

[kelunik]

@joshdifabio It's for implementing wait which continues the previous loop.

[joshdifabio]

@joshdifabio It's for implementing wait which continues the previous loop.

Of course, thanks.

[WyriHaximus]

@kelunik @davidwdan Looking at that last example the impact is actually minimal. And when looking at how I personally usually build up async (sub)projects this is a minor change. Tbh I doubt it will hinder adaptation much :smile:

[davidwdan]

I disagree. I think it will be a deterrent. React is over 90% of async php, so for all intents and purposes, is the current standard. This means that even without this change, there is little incentive for any react lib to adopt the interop loop.

[WyriHaximus]

You might actually be right about that :zipper_mouth_face: .

[kelunik]

@davidwdan In fact, the following code fails with the current code, which is quite surprising. It would fail with the PR merged.

// require autoloader
// setup factory

Loop::defer(function () {
    // never executed
});

Loop::execute(function () {
    // executes a new loop, the previous defer was registered on another loop
});

[kelunik]

I disagree. I think it will be a deterrent. React is over 90% of async php, so for all intents and purposes, is the current standard.

Sure, but React also doesn't have a global accessor currently, but passes the loop around.

This means that even without this change, there is little incentive for any react lib to adopt the interop loop.

For React libraries nothing will change, this PR doesn't affect libraries at all.

As React doesn't see real benefits in adopting the interop loop, they'll probably be covered using an adapter. It's up to them to provide an adapter the other way round then, any library / application using the interop loop can also use any React library by adding the adapter.

@WyriHaximus @cboden @davidwdan @mbonneau What do you think about the PR now? Could you change your reaction on the original post if your opinion changed or state a reason against this PR otherwise?

[kelunik]

Closing, as project discontinued for now.