[RFC] Global Loop #77

jsor · 2017-02-11T23:34:29Z

This is an alternative to #10 introducing a global loop on top of the current API and being backward compatible.

It adds a new API via functions on top of the current API for accessing the methods of the global loop (except run() and stop).

React\EventLoop\futureTick(function() {
});

This allows components to also introduce their own functions on top of their existing API using the global loop.

namespace React\Dns;

use React\EventLoop;

function resolver($nameserver)
{
    $factory = new React\Dns\Resolver\Factory();
    return $factory->create($nameserver, EventLoop\GlobalLoop::get());
}

By default, the driver is created by Factory::create(). This can be overridden by setting a custom factory using GlobalLoop::setFactory() which expects a simple callable returning a LoopInterface.

The global loop is run automatically in a shutdown function, so no need to call run() manually. This behaviour can be disabled by calling GlobalLoop::disableRunOnShutdown().

This is a RFC, so share your thoughts and comments :)

mbonneau · 2017-02-12T00:17:51Z

Consider running the shutdown function only if the loop has not yet been run. Otherwise, if the loop has been explicitly stopped, there may be events and timers that would resume on exit.

    public static function get()
    {
        if (self::$loop) {
            return self::$loop;
        }

        register_shutdown_function(function () {
            if (self::$disableRunOnShutdown || !self::$loop) {
                return;
            }

            self::$loop->run();
        });

        self::$loop = self::create();
        
        // disable auto-start if the loop gets run elsewhere
        self::$loop->futureTick(function () {
            self::$disableRunOnShutdown = true;
        });

        return self::$loop;
    }

jsor · 2017-02-12T21:28:40Z

@mbonneau When do you think a global loop should be stopped? In tests, you probably want to call disableRunOnShutdown() in the test bootstrap file to fully control the loop in the test methods.

mbonneau · 2017-02-12T21:45:32Z

@jsor I would suggest that the following code is not intuitive:

<?php

use React\EventLoop\Timer\Timer;

require __DIR__ . '/vendor/autoload.php';

$loop = React\EventLoop\GlobalLoop::get();

$timer = $loop->addTimer(1, function (Timer $timer) {
    $timer->getLoop()->stop();
});

$periodicTimer = $loop->addPeriodicTimer(0.25, function (Timer $timer) {
    echo "Doing something periodically.\n";
});

$loop->run();

Whether this is correct use of the event loop is debatable.

jsor · 2017-02-13T09:54:12Z

@mbonneau What is $timer->getLoop()->stop(); supposed to do here? Cancelling the periodic timer? If yes, the code should look like:

require __DIR__ . '/vendor/autoload.php';

$periodicTimer = React\EventLoop\addPeriodicTimer(0.25, function () {
    echo "Doing something periodically.\n";
});

React\EventLoop\addTimer(1, function () use ($periodicTimer) {
    $periodicTimer->cancel();
});

Maybe, React\EventLoop\run() and React\EventLoop\stop() should be removed so it's clearer that messing around with the global loop is dicouraged.

If you need to control the global loop, eg. in tests, you sould call disableRunOnShutdown()in your bootstrap script and use GlobalLoop::get()->run() / GlobalLoop::get()->stop() explicitely.

jsor · 2017-02-18T10:50:17Z

Just discovered reactphp/reactphp#330 again. This is basically the same i'm proposing here and this PR would be the foundation for implementing a functional API.

WyriHaximus

Loving this idea, lets make this happen 👍

WyriHaximus · 2017-02-22T10:45:39Z

README.md

-**Note:** The factory is just for convenience. It tries to pick the best
-available implementation. Libraries `SHOULD` allow the user to inject an
-instance of the loop. They `MAY` use the factory when the user did not supply
-a loop.


Could you add documentation how this is still the case when using the default factory. And also documentation how this change would work in general.

This RFC ist just a POC to illustrate the idea and is not meant to be merged as is. If this is going to happen, we must if course add additional documentation, not only for how the factory works.

Yes of course, but looking at this POC I'de like, either here or in another PR, get started working this out. My preference would be on this PR though.

What i meant was: i surely add docs, but only after the other reviewers have not rejected this RFC as pure non-sense :P

Yes I gathered that 👍

WyriHaximus · 2017-02-22T10:47:04Z

composer.json

@@ -17,6 +17,7 @@
    "autoload": {
        "psr-4": {
            "React\\EventLoop\\": "src"
-        }
+        },
+        "files": ["src/functions.php"]


Personally I'm fan of using an intermediary functions_include.php like this, this avoids weird edge cases when using threads:

https://github.com/WyriHaximus/TickingPromise/blob/master/composer.json#L25

https://github.com/WyriHaximus/TickingPromise/blob/master/src/functions_include.php

Of course, thanks for the reminder!

@WyriHaximus Which edge cases exist there? Could you elaborate?

See reactphp/promise#23 and reactphp/promise#25. Might be fixed in the meantime by composer/composer#4186.

WyriHaximus · 2017-02-22T10:47:57Z

src/GlobalLoop.php

+     *
+     * @var LoopInterface
+     */
+    public static $loop;


Why not always force GlobalLoop::get? Making this public could potentially mean someone swaps it out and breaks everything.

This is currently public for 2 reasons:

It allows to save a function call in the function API, see eg. https://github.com/reactphp/event-loop/pull/77/files#diff-84b928a9905378810b602c8a740b4cbaR16

It allows to swap out the loop, see eg. https://github.com/reactphp/event-loop/pull/77/files#diff-6872a075db159cbbcfc073b693c7d460R18

Note, that the property is marked @internal, so it should be pretty clear the messing around with it isn't a good idea ;)

Again, this is just a POC, so we might decide to make it private, always use GlobalLoop::get() and add a corresponding GlobalLoop::set() for swapping out the loop, eg. in tests.

Makes sense, lets see what @clue, @cboden, and @nrk have to say and get the ball rolling if they like it 😄

@jsor @WyriHaximus This is one reason why https://github.com/async-interop/event-loop uses a static class instead of functions and has the clear Loop::execute scoping. The current loop can be a private property and swapping out the loop is very easy and not something you have to worry about.

joshdifabio · 2017-02-22T15:55:23Z

I'm curious about the decision to use functions here instead of static methods on the Loop class. The functionality in those functions is very much dependent on the state of GlobalLoop.

jsor · 2017-02-22T19:57:47Z

@joshdifabio Apart from React\EventLoop\addTimer() being nicer than React\EventLoop\GlobalLoop::addTimer(), it should make the distinction between the API for application and infrastructure/library code.

The functional API targets applications.

React\EventLoop\futureTick(function() {
});

React\Dns\resolve('reactphp.org')->then(function($ip) {
});

The GlobalLoop API targets infrastructure code/libraries.

// bootstrap file
React\EventLoop\GlobalLoop::setFactory($factory);

namespace React\Dns;

function resolve($name)
{
    $factory = new React\Dns\Resolver\Factory();
    return $factory->create('8.8.8.8', React\EventLoop\GlobalLoop::get())->resolve($name);
}

joshdifabio · 2017-02-23T11:44:26Z

@jsor I don't agree with the rationale behind using functions, but I appreciate the time taken to explain. Thanks.

How do you guys see this interoperating with the interop standard?

jsor · 2017-02-23T11:58:05Z

@joshdifabio

@jsor I don't agree with the rationale behind using functions, but I appreciate the time taken to explain. Thanks.

Since this is a RFC, i'm really interested in your thoughts/comments on this regarding a better API.

How do you guys see this interoperating with the interop standard?

Something like this probably:

React\EventLoop\GlobalLoop::setFactory(function() {
    return new WyriHaximus\React\AsyncInteropLoop\AsyncInteropLoop();
});

kelunik · 2017-02-24T08:34:52Z

If you really make the change and use a global accessor, why not directly support async-interop? I can see that it makes no sense to support async-interop when you don't have a global accessor, but why have your own?

I can also only strongly advise against running an entire application in a shutdown function. Any exit will directly exit the whole application instead of running other shutdown hooks as expected.

kelunik · 2017-02-24T08:37:25Z

src/GlobalLoop.php

+        if (self::$loop) {
+            throw new \LogicException(
+                'Setting a factory after the global loop has been created is not allowed.'
+            );


How can it be reset to work in tests?

kelunik · 2017-02-24T08:57:09Z

Apart from React\EventLoop\addTimer() being nicer than React\EventLoop\GlobalLoop::addTimer(), it should make the distinction between the API for application and infrastructure/library code.

Why should libraries use the API differently than application code?

Note, that the property is marked @internal, so it should be pretty clear the messing around with it isn't a good idea ;)

@jsor It's marked as internal, but is basically required by all test suites to fiddle around with? Doesn't seem like a good API decision then.

jsor · 2017-02-24T20:59:54Z

In our components, the loop instance gets injected. They do not use the global loop directly and thus do not depend on the global loop in tests. Only the thin functional layer on top, which basically just does some wiring, accesses the gobal loop.

jsor · 2017-03-11T10:11:24Z

I've filed #82 as a simplified alternative.

…t-loop#82 or reactphp/event-loop#77 is merged

WyriHaximus · 2021-06-06T17:07:38Z

Closing this as it has been resolved through #226 and follow up PR's

jsor added 2 commits February 12, 2017 00:09

Add a global loop with accessor functions

8cc748e

Automatically run global loop

9b893c3

jsor added 2 commits February 13, 2017 19:47

Merge branch 'master' into global-loop

6401dd9

Remove global run() and stop() functions

8f5d8ad

jsor force-pushed the global-loop branch from bf6d084 to 8f5d8ad Compare February 13, 2017 18:50

jsor added help wanted new feature labels Feb 17, 2017

WyriHaximus requested review from nrk, cboden and clue February 22, 2017 08:28

WyriHaximus requested changes Feb 22, 2017

View reviewed changes

kelunik reviewed Feb 24, 2017

View reviewed changes

jsor mentioned this pull request Mar 11, 2017

[RFC] Global Loop (simplified) #82

Closed

WyriHaximus added a commit to php-api-clients/pusher that referenced this pull request May 24, 2017

Rather not set these factories but have to until either reactphp/even…

9148a5a

…t-loop#82 or reactphp/event-loop#77 is merged

WyriHaximus mentioned this pull request Apr 21, 2021

Global event-loop accessor part four #226

Merged

WyriHaximus closed this Jun 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Global Loop #77

[RFC] Global Loop #77

jsor commented Feb 11, 2017 •

edited

Loading

mbonneau commented Feb 12, 2017

jsor commented Feb 12, 2017

mbonneau commented Feb 12, 2017

jsor commented Feb 13, 2017

jsor commented Feb 18, 2017

WyriHaximus left a comment

WyriHaximus Feb 22, 2017

jsor Feb 22, 2017

WyriHaximus Feb 22, 2017

jsor Feb 22, 2017

WyriHaximus Feb 22, 2017

WyriHaximus Feb 22, 2017

jsor Feb 22, 2017

kelunik Mar 4, 2017

jsor Mar 4, 2017

WyriHaximus Feb 22, 2017

jsor Feb 22, 2017

WyriHaximus Feb 22, 2017

kelunik Feb 24, 2017

joshdifabio commented Feb 22, 2017 •

edited

Loading

jsor commented Feb 22, 2017

joshdifabio commented Feb 23, 2017

jsor commented Feb 23, 2017

kelunik commented Feb 24, 2017

kelunik Feb 24, 2017

kelunik commented Feb 24, 2017

jsor commented Feb 24, 2017

jsor commented Mar 11, 2017

WyriHaximus commented Jun 6, 2021

[RFC] Global Loop #77

[RFC] Global Loop #77

Conversation

jsor commented Feb 11, 2017 • edited Loading

mbonneau commented Feb 12, 2017

jsor commented Feb 12, 2017

mbonneau commented Feb 12, 2017

jsor commented Feb 13, 2017

jsor commented Feb 18, 2017

WyriHaximus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdifabio commented Feb 22, 2017 • edited Loading

jsor commented Feb 22, 2017

joshdifabio commented Feb 23, 2017

jsor commented Feb 23, 2017

kelunik commented Feb 24, 2017

Choose a reason for hiding this comment

kelunik commented Feb 24, 2017

jsor commented Feb 24, 2017

jsor commented Mar 11, 2017

WyriHaximus commented Jun 6, 2021

jsor commented Feb 11, 2017 •

edited

Loading

joshdifabio commented Feb 22, 2017 •

edited

Loading