Global event-loop accessor part four

[WyriHaximus]

This topic has come up before in #10, #77, and #82. And their aim was to provide all the pieces in one go, the aim for this PR, and its successors in line, is to discuss all bits in their own PR. As such this PR is providing the bare minimum.

To help that I've made several examples to provide different angles on this topic:

• Full switch over to the new global loop accessor: Switch to use the global event-loop WyriHaximus/reactphp-cron#51
• Make passing the loop optional, but as the recommended way: Make loop optional clue/reactphp-redis#110
• Fully switch over to the global loop accessor, including setting a new loop for each test: Use global loop accessor clue/reactphp-redis#109

Resolves: #10 #77 #82

[mbonneau]

Should Loop::set allow setting the loop if it has already been set?

What do you think about registering a shutdown function to run the loop?

[WyriHaximus]

Should Loop::set allow setting the loop if it has already been set?

Yes, but only when it hasn't been started yet. The issue with this is testing of course so not 100% sure yet.

What do you think about registering a shutdown function to run the loop?

I'm open for it, but not sure if it's wise thing to do. Have you done this in your projects, and how did it work out?

[cboden]

I'm open for it, but not sure if it's wise thing to do. Have you done this in your projects, and how did it work out?

Matt added this to Pawl. It's been a great quality of life improvement.

[mbonneau]

In https://github.com/voryx/event-loop, we have been using the register_shutdown_function the same way it is implemented in Pawl. We have not experienced any problems with the shutdown function usage. This library is used in almost every php project that I have worked on in the last 5 years or so. I am definitely an advocate of that being included in the official library.

We tried to only expose things from the library that made sense from the user standpoint. So for testing we used reflection to reset the private static property. (https://github.com/voryx/event-loop/blob/master/tests/Unit/EventLoopTest.php#L11)

[WyriHaximus]

In https://github.com/voryx/event-loop, we have been using the register_shutdown_function the same way it is implemented in Pawl. We have not experienced any problems with the shutdown function usage. This library is used in almost every php project that I have worked on in the last 5 years or so. I am definitely an advocate of that being included in the official library.

Cool, but I really would prefer to do this in several smaller PR's than one big one.

We tried to only expose things from the library that made sense from the user standpoint. So for testing we used reflection to reset the private static property. (https://github.com/voryx/event-loop/blob/master/tests/Unit/EventLoopTest.php#L11)

That makes sense, would you think it makes sense to provide a utility for it?

[trowski]

Running the event loop in a shutdown function is not a good idea. The engine has moved to shutdown mode and executing an entire app within that mode can lead to subtle breakages: https://heap.space/xref/php-src/main/main.c?r=39ddf6b8#1772

I thought of this long ago, and on the surface it seems to work well, but is a bit of a hack.

[HLeithner]

With this implementation it's not possible to detect if an event loop already exists,

This could be useful in scenarios with 3rd party plugins and mixed workflows (with and without event loop, sync / async flow).

I'm not 100% sure that it's necessary. ymmv

[clue]

@WyriHaximus Thanks for filing this PR among with clue/reactphp-redis#109 and clue/reactphp-redis#110, this definitely helps reviewing and seeing its real-world impact on existing code!

I think we all agree that having a global loop makes perfect sense and is a real improvement towards a more streamlined API for consumers of any ReactPHP-based project! It also looks like this PR is an excellent starting point to discuss the minimum required to get this rolling.

That being said, I'd like to ensure we have a smooth upgrade path for existing consumers and a clear understanding of the consequences of this approach.

The suggested changes help making any LoopInterface arguments optional throughout the ReactPHP ecosystem. As a consequence, this means that most consumers will no longer create and/or pass the loop around to all ReactPHP components.

I agree with this suggestion and think this is a great first step. I think there's definitely room to improve this even further, but I suppose the goal seems to be to discuss this in separate tickets? (additional helper methods, automatic shutdown handler, etc.)

Other than that, have you checked how this works together with existing code still using the loop factory? It's my understanding both styles will be mixed at least during a transition period, so I'd like to see some additional safeguards in place. If this is taken care of, consider my vote a strong 👍

[WyriHaximus]

@clue Updated the PR as discussed earlier today during our zoom call. Factory::create will now call Loop::set with the created event loop. (Had to do a small refactor.) Also marked Loop::set as internal.

@HLeithner The changes I've just pushed should address your concern. As long as you call Factory::create before Loop::get you'll be fine. Also during mixed workflows, you can now use Loop::get and you'll always get the same loop back. That means if you also use https://github.com/clue/reactphp-block you might have leftovers in your loop from your previous run if you didn't clean up. Highly doubt that is going to be an issue, unless it's used in a messy way that leaves a lot of garbage between runs.

[WyriHaximus]

Running the event loop in a shutdown function is not a good idea. The engine has moved to shutdown mode and executing an entire app within that mode can lead to subtle breakages: https://heap.space/xref/php-src/main/main.c?r=39ddf6b8#1772

I thought of this long ago, and on the surface it seems to work well, but is a bit of a hack.

@trowski What kind of issue did you run into?

[trowski]

@trowski What kind of issue did you run into?

Some behaviors are modified during shutdown. An uncaught exception or fatal error from a shutdown function will not run other shutdown functions. This differs from an uncaught exception in {main}, which will run shutdown functions.

Consider the following:

Loop::get()->nextTick(fn () => throw new \Exception('test'));

register_shutdown_function(fn () => print "shutdown 1\n");
register_shutdown_function(fn () => Loop::get()->run());
register_shutdown_function(fn () => print "shutdown 2\n");

The first shutdown function will be invoked before the loop is started. Upon entering the loop, an exception is thrown from a loop callback, resulting in an uncaught exception. However, the second shutdown function is never invoked.

In short, conditions that trigger a bailout in a shutdown function differ in behavior than a bailout from {main}.

The above code also demonstrates how defining a shutdown function becomes timing dependent, and may conflict with libraries that define shutdown functions when loading code or user application code that defines a shutdown function before starting the loop.

Entering shutdown can have unexpected behaviors in certain extensions that may use EG(flags) to determine execution state.

While starting the loop in a shutdown function works on first glance, it is my opinion that the loop package should not endorse this practice due to the potential change in execution and shutdown behavior. Auto-starting the loop in a shutdown function could be available in a supplementary package (with potential caveats explained), but I would not suggest it as the official method to run the event loop.

[mbonneau]

@trowski Firing off the loop inside the register_shutdown_function is a hack. But I think it is worth it. It is just so much more ergonomic for new reactphp users.

There are libraries that use register_shutdown_function that may not work properly with this use, but I would hazard a guess that it is far less than 1% of all PHP libraries.

Users can also completely mitigate this problem, if needed, by running the loop themselves, which would cancel the run inside the shutdown function.

[clue]

@WyriHaximus Thanks for the update!

The changes look good to me, but I'd like to check if we can add some additional tests and address the minor remarks below.

Other than that, I think this is ready to go in 👍

Regarding the automatic running of the loop (possibly with the help of a shutdown handler), I've discussed this matter with @WyriHaximus and we've agreed that this is out of scope for this PR. We've also agreed that this should be addressed in a separate PR as part of the same milestone to avoid any unexpected regressions in upcoming versions, so this will be addressed shortly.

Once merged, I'll pick this up and see how reasonable automatic running is 👍

[WyriHaximus]

@clue Updated the PR based on your comments. Added several tests and the requested docblock.

[kelunik]

It seems strange to me to make this method internal. Users can then no longer decide to use a specific event loop implementation without using internal APIs, because the factory is the only public API, e.g. if both uv and ev are installed, a user might still prefer to use ev.

[WyriHaximus]

Until we are sure about what such behaviour should be of the Loop::set method it will be marked as internal, and undocumented. You can still use it, but at your own risk. We don't want to introduce a function without being absolutely sure it is the right thing to do. Because once we add it, we can't remove it until the next major version

[clue]

Users can then no longer decide to use a specific event loop implementation without using internal APIs […]

Any event loop can still be instantiated as usual if you need to make an explicit choice, this PR does not change anything about this. For the >99% use cases only a single event loop implementation would be available and the default choice is still reasonable.

It seems strange to me to make this method internal.

I agree that this may appear odd on first sight. Like @WyriHaximus pointed out, we're using this as a starting point to be able to gather more feedback. Expect a public set() method not too far in the future in a separate PR

[kelunik]

This side effect of a factory is pretty much unexpected IMO.

[WyriHaximus]

Main reason for adding that was to support mixed use cases where both the Factory and the Loop object are used.

[clue]

This side effect of a factory is pretty much unexpected IMO.

I agree that this may appear odd on first sight. Like @WyriHaximus pointed out, we're using this as a starting point to be able to gather more feedback. Right now this has no effect on any consumer because the Loop::set() is entirely internal only and the Loop::get() uses the very same Factory::create() method. Expect a cleaner approach not too far in the future once we have a public Loop::set() method in a separate PR