async-interop / event-loop Goto Github PK

Following the Hangout of Async Interop vs. Fig inclusion: is it possible to force synchronous execution through these interfaces. I'm not asking this well, I think, but would it be possible to enter some always execute immediately mode of event loops? I'm going to ask the same question when we get to promises, but it probably needs to be asked here first.

Should calling a watcher method on a stopped loop throw an exception?

I'm not sure what behavior makes sense in this situation. It may make sense to create an initial set of watchers, then start the loop. I can also see where it could be a mistake to create a watcher in a loop that is not running, so throwing would be preferred.

My initial thoughts are to allow creating a watcher in a stopped loop under the assumption that it will be started. With the current interface this would have to be the case, as Loop::execute() executes the callback provided, then calls LoopDriver::run().

For watcher methods to throw exceptions from a stopped loop, LoopDriver::run() would have to accept a callback, start the loop, then execute the callback.

In the name of keeping our discussions in nicely structured issues...

@trowski proposed using watcher identifiers in an initial event loop proposal here.

I think it would be a good use of the separation of concerns principle to instead create a Watcher object that contained ::enable(), ::disable() and ::cancel() methods.

All thoughts and opinions please!

We've pretty much cleared the issue and pull request queue now.

One of the tasks we were saving to this point is the line length on the phpdoc and when to wrap. I'm happy to go with any limit as long as we are consistent and it's not ridiculous (I can read it on GitHub without scrolling or wrap).

Do we really need a common standard which then gets implemented multiple times? I think a common implementation makes much more sense. What do you think?

I hereby propose not to follow PSR-2 (completely). I'd like to have opening braces always on the same line, also for classes, methods and functions.

Already multiple people expressed they don't like that part of PSR-2. (That doesn't mean they expressed support for this proposal)

• bwoebi
• assertchris

Summary of discussion so far:

@assertchris prefers AMP's once() and repeat()
@trowski prefers Icicle's timer() and periodic() but says that once() and repeat() would also be acceptable.
@WyriHaximus really likes AMP's once() and repeat() but also suggests timer() and periodicTimer()

I'm going to make things difficult, as I think that once() could quite easily be confused with queue() - as this is also something that is run "once".

Indeed, for AMPs interface they currently have:

public function immediately(callable $callback, array $options = []);
public function once(callable $callback, $msDelay, array $options = []);

I think what their interface would be clearer it used delay, as this is the difference between the two methods.

public function immediately(callable $callback, array $options = []);
public function delay(callable $callback, $msDelay, array $options = []);

I think this also raises a question about whether we use queue() or immediately(). For me these have different meanings, queue meaning we'll get around to it and immediately meaning the next thing that happens. Which here is more accurate?

My current vote would be for delay() and repeat() - interested to hear your thoughts!

Currently, there's nothing that describes when the error handler is invoked and when the error couldn't be handled by the error handler. My guess is that the error handler should rethrow if it can't handle the exception and otherwise do nothing. Return value doesn't matter.

Shall we really go by EventLoop or can we shorten the classnames to just Loop?

I've found it's not less meaningful, is more concise/easier to type and read.

This is obviously mainly a subjective thing... Feel free to disagree, just my personal opinion while looking at https://github.com/async-interop/event-loop-playground/blob/11b3492f5a78f53167cd4ebb54d9566c1fcb7263/run.php ... feels cleaner to me to have it called just Loop.

All operations like disable, enable, cancel with an invalid (non-existent) watcher ID MUST throw an exception. We need to add it to the phpDoc and create a base exception for that.

This is a new issue to aggregate some discussion that happened in #14 and #17 concerning how the global event loop instance is controlled.

Currently we are offering Loop::execute() to set the loop driver and Loop::get() to fetch the instance. Is this the best approach, or should we also provide a get()/set()-style control as well? We discussed this a bit previously, but I wasn't sure if we came to a conclusive stance.

Here's a summary of the arguments so far that I recall:

• Provide just execute() and get(): Eliminates the possibility of people messing with loop instances in ways they shouldn't, provides clear scoping at the entry of the program.
• Provide get(), set(), and drop(): Greater flexibility, nesting in a closure is not mandatory.
• Provide all or combination of above methods: Kind of all above benefits, except you lose guarantee that people can't break things and you now have multiple ways of doing a similar thing.

I closed my old PR as the purpose had already been solved: adding a static class for storing the global driver instance. I opened a new PR (#27) with a reference implementation for discussion.

Let's open this can of worms!

Do we (or could we) need or want any of the features in PHP 7?

This is a topic that I've discussed independently with a few people here, and I'm still not sure that I'm 100% convinced by any of the solutions.

I guess the best phrasing of the problem is:

"Most PHP application instances will require no more than a single event loop. How can we ensure that this event loop can be easily accessed by code within the application?"

From here I believe there are a few options (feel free to add):

1. Straight forward dependency injection. The benefits of this is that it's explicit, you can see immediately whether a class uses the event loop and you have type safety. The negative is that it's repetitive and requires a lot of boiler plate code.
2. Make the event loop global. Have a class that can act as a locator with static properties (or functions) so that code anywhere in the application can retrieve the instance of the event loop.
3. Some hybrid between these, which allows users to choose whether they wish to explicitly require the event loop as a class dependency or just access it statically when they need it.

A big factor at play here is testability. I'm not convinced that anything other than option 1 is a good bet for writing testable code - but I have been told that the other options don't interfere with this. Has anyone got an example of well tested classes that retrieve their instance of the event loop from global scope? I'm happy to be convinced on this point :)

Another question I have is - can anybody think of a use case where a single PHP instance would wish to have multiple event loops operating at any one time?

Interested to hear your thoughts,

Andrew

Amp has Amp\info to inspect the loop. This can be useful for metrics or debugging.

https://github.com/amphp/amp/blob/master/lib/Reactor.php#L138-L160

I guess it would be fine having them listed in the README. Suggestion comes from https://www.reddit.com/r/PHP/comments/4t6ggs/first_draft_version_of_the_eventloop/d5ezhqg

Icicle

interface Loop
{
    public function tick(bool $blocking = true);
    public function run(callable $initialize = null): bool;
    public function stop();
    public function isRunning(): bool;
    public function clear();
    public function reInit();
    public function maxQueueDepth(int $depth = null): int;
    public function queue(callable $callback, array $args = []);
    public function poll($resource, callable $callback, $persistent = false, $data = null): Io;
    public function await($resource, callable $callback, $persistent = false, $data = null): Io;
    public function timer($interval, $periodic, callable $callback, $data = null): Timer;
    public function immediate(callable $callback, $data = null): Immediate;
    public function signal($signo, callable $callback, $data = null): Signal;
    public function signalHandlingEnabled(): bool;
}

React

interface LoopInterface
{
    public function addReadStream($stream, callable $listener);
    public function addWriteStream($stream, callable $listener);
    public function removeReadStream($stream);
    public function removeWriteStream($stream);
    public function removeStream($stream);
    public function addTimer($interval, callable $callback);
    public function addPeriodicTimer($interval, callable $callback);
    public function cancelTimer(TimerInterface $timer);
    public function isTimerActive(TimerInterface $timer);
    public function nextTick(callable $listener);
    public function futureTick(callable $listener);
    public function tick();
    public function run();
    public function stop();
}

AMP

interface Reactor
{
    const STOPPING = -1;
    const STOPPED  = 0;
    const STARTING = 1;
    const TICKING  = 2;
    const RUNNING  = 3;

    public function run(callable $onStart = null);
    public function tick($noWait = false);
    public function stop();
    public function immediately(callable $callback, array $options = []);
    public function once(callable $callback, $msDelay, array $options = []);
    public function repeat(callable $callback, $msInterval, array $options = []);
    public function onReadable($stream, callable $callback, array $options = []);
    public function onWritable($stream, callable $callback, array $options = []);
    public function onSignal($signo, callable $func, array $options = []);
    public function onError(callable $callback);
    public function cancel($watcherId);
    public function disable($watcherId);
    public function enable($watcherId);
    public function info();
}

Shall we throw an exception here? Or rather not?

Having it unspecified is probably not the best idea.

I prefer throwing here.

Should we go with Interop\Async as namespace instead of Interop\Async\EventLoop?

I created a promise draft, using Interop\Async\Promise\Promise seems bad and as we just have a few interfaces, I think it'd be fine to use just Interop\Async as namespace for both.

Just noticed that the phpdoc says the feature class constants are on the static proxy (https://github.com/async-interop/event-loop/blob/master/src/Loop.php#L217) but they are actually defined on the loop driver.

I think the phpdoc is right here, but there is discussion to be had (it could even be on both or a separate interface).

Just like one off and periodic timers you might want to cancel it at some point

Let's discuss a seperate standards body, for async interoperability, here.

What exactly is the purpose of tick() except leaking the internal detail of being "ticked"?

A tick is easily simulated with (using Amp's interface):

$reactor->immediately(function() use ($reactor) {
    $reactor->immediately(function() use ($reactor) {
        $reactor->stop();
    });
});
$reactor->run();

It's a nice helper function, but does it really have to be directly accessible? Also, the only usage for tick()'s I've seen so far is to wait for a single promise, which can be also written as:

$promise->when(function() use ($reactor) {
    $reactor->stop();
});
$reactor->run();

We cannot declare a type for the $stream parameter in onReadable and onWritable. Which exception should be thrown if there's an invalid argument passed? Should we add a new InvalidStreamException? Maybe an own (more generic) InvalidArgumentException? Should we use the one of Spl instead?

First of all, if it's been categorically decided that PHP 5.5 should be supported by these specs then feel free to close this issue, since I'm proposing a change which makes use of a PHP 5.6 feature. However, considering 5.5 will be EOL in one month's time, I'm not convinced it should be supported by a spec which will most likely be ratified after that date.

If people are willing to consider bumping the minimum PHP version to 5.6, I propose that the $data = null param be replaced with ...$data in all methods within Loop which use the $data param, and that the splat operator also be used to extract the provided $data when invoking callbacks.

Consider the following (admittedly fairly weird) examples:

Possible approach with current interface

Loop::defer(function (string $watcherId, array $data) { // no type safety for contents of $data
    $data['repository']->save($data['user']); // easy to make a typo when typing array keys
}, ['repository' => $repository, 'user' => $user]); // have to create an array here which is suboptimal

Possible approach with proposed interface

Loop::defer(function (string $watcherId, Repository $repo, User $user) { // type safety
    $repo->save($user);
}, $repository, $user);

I realise the use case here is fairly contrived and doesn't make loads of sense, but hopefully it demonstrates the benefits of using splat over a single $data param.

Hi All,

It's popped up in other discussion threads about whether cancel() means "permanently disabled" or "deleted".

I can see how this confusion has come in, but I think there is an obvious case for the latter - and that we should rename the method to remove any doubt.

Without a method which performs the operation of freeing the resources associated with the loop by the driver, there is no way than an application using the loop can guarantee that it isn't leaking memory without destroying the loop object. For use in a long running application, it's a requirement that there is an operation which can be used to free these resources - thus it can not be considered an implementation detail.

Let the discussion begin!

Apologies for my recent absence - I've had an obscene amount of work over the last few weeks and I'm not even close to the end. Regardless, I've had some concerns that can't be delayed any longer as (happily) we're progressing on with this.

My main issue is a personal U-turn in opinion regarding using ::execute() as a way to give scope to the static proxy methods. The code below is an attempt to illustrate the problem - which is that no service can safely and easily perform watcher operations. This is because there is no guarantee that the currently active loop driver is the one that issued the watcher. In the example below, the two ping calls could easily occur in different loops and cause a problem.

class Client
{
    private $stream;
    private $watcher = null;

    public function __construct($stream)
    {
        $this->stream = $stream;
    }

    public function ping()
    {
        if ($this->watcher !== null) {
            // This line could screw up
            Loop::cancel($this->watcher);
        }

        $this->watcher = Loop::onWritable($stream, function () use ($stream) {
            fwrite($stream, 'ping');
        });
    }
}

Loop::execute(function () {
    $client = new Client(...);

    $httpClient->get('/some-resource')->success(function () use ($client) {
        $client->ping();
    });

    // some stuff

    $client->ping();
});

The Client could store the watchers in the loop registry, but this is going to make the code very ugly, very quickly. Given that the purpose of this design is to avoid dependency injection with the hope of simplifying, I'd argue that we should re-evaluate if the current approach actually helps to remove complexity at all.

Although I could be persuaded, currently I think I would prefer ditching the registry and using ::set(), ::get(), ::clear() (or whatever) and making it so that consumers must manage the state of the loop themselves.

This leads on to my second issue, which is that we sort of accidently already have ::set() because calling ::setFactory() then any of the proxy methods will automatically create a loop and activate it - despite not being within the bounds of an :execute() call. I'm not really sure I have a strong opinion on this - it just feels like there's no clear way to use the API that we currently have.

I think there should only be one possible onError handler. It should be set be the application only, never by libraries. Currently we return an event identifier as well and can cancel it.

@trowski and @rdlowrey already agreed in chat, but just confirm that with a 👍

The Uv extension exposes functions for watching on signals and to execute operations on files.
Other loops may expose signal handling through pcntl.

a) I assume signal handlers will just throw an exception if you try to register one via onSignal() while unsupported? … Do we want to expose a way to actually access that information on whether it is supported without trying and catching?
b) In general loops are handled by one underlying loop resource (uv, ev) - do we want to be able to expose what type of loop exactly and the underlying loop actually is?

Regarding b), in Amp, it is simple, we have three implementations and can just check for the actual instance and have one function getLoop() (not in the interface) on the UvReactor. This can be used by our amphp/file library to actually operate on it.

But if we are going to have multiple implementations, we need to have a way to access that data (loop type, loop resource if existing).

I'm not really sure how to shape that though? Maybe a function getLoop() returning an array [$extension, $loop] … so that it would be ["uv", $the_uv_loop] or ["native", null]?
I'm really open to any better suggestion here!

Many of the underlying event loop libraries support a timeout when polling a socket. Do we want to expose this capability in read/write watchers to avoid having to create a separate timer?

We currently aren't coupled to any promise / coroutine / whatever API. This means that we can't use coroutines as an argument to Loop::execute. It might be possible to achieve it using something like Amp\coroutine.

My actual question is: How are we going to write tests? Using the an emulated Loop::wait / a decoupled wait function? The reason is that we need the thing that executes to throw exceptions thrown inside the callable to outside of the callable, so we can use things like PHPUnit's @expectedException.

This issue relates to #11. Looks like this vote doesn't pass: https://groups.google.com/d/msg/php-fig/S-URR0_h2U0/1hpJhkgiDwAJ. If we want this standard to be a FIG standard, we have to adjust our class names.

• Driver → AbstractDriver
• DriverFactory → DriverFactoryInterface

We also might want to follow other PSRs and use exception interfaces instead of classes. We might even want to do that independently.

I noticed that for both delay() and repeat() the callback param comes before the time/interval param. This is the same as Amp's reactor interface but different to Icicle and React's event loop interfaces which both place the time/interval param first. The current approach results in code like the following:

$reactor->delay(function () {
    //lots
    //of
    //code
    //here
    //...
    //...
}, 1000);

versus the alternative

$reactor->delay(1000, function () {
    //lots
    //of
    //code
    //here
    //...
    //...
});

The latter seems much easier to grok, which I noticed when re-factoring some code from React\EventLoop today. Has this been discussed or are people pretty happy with the Amp ordering of params?

Note that onReadable, onWritable and onSignal all require the stream/signal nr. params before the callbacks in the current interface, so it's not like the callback is always placed first for consistency reasons.

Current opinions from other threads:

@bwoebi #6 (comment)

@AndrewCarterUK why are you proposing floats for times? The precision of the reactors/loops is one millisecond. (Some do allow for more, but e.g. for uv it's at most one millisecond. And we should be able to get same results for each backend.) At least that's the reason why Amp choose to use milliseconds as base unit and should be retained here.

@trowski

@bwoebi Perhaps someday there can be greater precision than 1 ms. Using a float allows for that.

@bwoebi

@trowski If there will ever be, it will be a BC break (significant enough for a major I mean) anyway as currently we should guarantee that all the backends use the same granularity for time management and that granularity would be changed too.

All major contributors should be maintainers IMO, see https://packagist.org/packages/async-interop/event-loop. @assertchris @WyriHaximus @trowski @AndrewCarterUK

Currently our exceptions extend LogicException and RuntimeException. I'd like to rather not use SplExceptions but rather extend Exception directly.

I think we should change the names to setState and getState. What do you think?

I've been running into timing issues causing Loop::disable() to throw due to a watcher being invalidated during the invocation of the watcher callback. Essentially, if the watcher callback calls Loop::disable() at the end of the callback after side-effects from the watcher callback have invoked Loop::cancel(), an exception is being thrown. While it is possible to use an additional flag to code around this problem, it seems unnecessary.

Some example code: https://github.com/amphp/socket/blob/amp_v2/lib/Server.php#L37-L51

Putting the call to Loop::cancel() in a defer watcher seems to solve the issue here. I tried the same in the Socket class within the same lib here. However, this does not appear to have solved the problem, as tests still occasionally result in throws due to invalid watchers.

I propose that InvalidWatcherException should not be thrown from disable() with cancelled watchers, as disabling a cancelled watcher does not change the expectation that further events will not be received on the watcher. enable() should continue throwing when a cancelled watcher is given.

I think it would be better to merge Registry into Driver and make Driver an abstract class and those two methods final.

Wait could be implemented like this:
https://gist.github.com/kelunik/4163c84839ce374a173e57926a608b58

Issue is that we would either have to make when throw under certain conditions or swallow the exception until the loop is continued, e.g. by the next wait, see http://chat.stackoverflow.com/transcript/message/30615565#30615565

Should we drop Loop::supports and rely only on the thrown exception?

Usually, if you need signal handling, you will error out if you don't have it available anyway. If you need if ... else anyway, we can also use try ... catch directly.

There seems to be consensus to use a factory for the loop (see #30).

The open issue is whether we shall use a Factory interface or require a callable.

Currently, with 5.5 neither are actually hintable against a return type - but in future we may also have callable hinting.
I don't see any particular advantage of requiring an interface here.

Thus, as I think callables are more approchable and easier to implement [and especially the functions don't have to be reusable in this case], a callable should be preferable in this particular case.

As we approach stabilization of the API...
Do we want to provide tests here which every implementation must pass?

This would ensure that implementations are compatible between each other, at least on the level of the tests.

In case we can agree here, I'd take the Amp tests and update them to match the Loop API. (as differences are rather small). This would be then an abstract static class with an abstract function to get the respective Driver instance.

Note: Tests are not required for a first v1.0.0, they may still be included in a minor version later...

We already have methods for detecting when streams have readable or writeable data - but you can also poll for 'exceptional conditions' on a stream (I'm thinking POLLPRI).

In a stream_select call this is the $except parameter.

I'm currently doing some work on the Raspberry Pi which requires this for interrupt handling (this event is triggered when the voltage changes on a pin to simulate an interrupt). Though I know of no examples, I'd imagine this is a common technique for simulating interrupts in userland.

What are our thoughts? Are there any reasons not to add this?

Sorry, I should have noticed this when the original discussion came up to shorten the namespace.

Currently we have the UnsupportedFeatureException inside the Interop\Async namespace, which is obviously not ideal.

Should we wish to produce another standard that required a similar exception there would be a collision, or we would have to break consistency and things would be a mess.

I can see the following solutions:

• Change the namespace back to the longer form
• Create a Loop directory and move the driver and exception classes into there

I'm probably favouring the second option now, that would leave us with the following paths:

Interop\Async\Loop
Interop\Async\Loop\Driver (or LoopDriver)
Interop\Async\Loop\UnsupportedFeatureException

Interested to hear your thoughts.

From #53:

May we please add that cancel()'ling multiple times or any other operation on already invalid watchers is disallowed (as watcher ids may be reused)?
Calling unreference(), reference(), enable() and disable() multiple times must not error.

Is everyone okay with that (I'll open a PR tomorrow to add that in docblocks)? Or are there issues here?

When exactly becomes a defer()/once() watcher id invalid? Before the callback call (as you anyway cannot do meaningful disable/enable on it inside the callback) or only after it?

This needs to be specified in the Driver interface.

Moved from #14 (comment).

We need some mechanism to store global state bound to a loop. This can be used for things like a default DNS resolvers or filesystem drivers.

Since we have I/O bound applications, most operations will have something to do with networking operations. Most of these need DNS, so it might make sense to have a global accessor for DNS just like for the event loop itself.

Otherwise it's possible to pass it optional and create a new one if no resolver is specified. Multiple instance aren't a problem for DNS, just for the event loop.

As already written to @AndrewCarterUK in private:

e.g. you open a DNS connection and store the connection globally, because DNS is something you want globally in your application, because not every socket should have to deal with an injected resolver. If you swap the event loop now and make another DNS lookup, it will use the same connection, but will never receive a read event, since the event loop that could trigger that read isn't currently running.

If the connection is bound to the loop, it would just create a new connection once the loop is swapped.

We need to define the callbacks, currently there's just everywhere callback, but no signature.

Should we add the word 'Interface' to the end of the interface class paths?

PSRs 3, 6 and 7 do (except in PSR-6 for the exception interfaces).

Created an issue for this as I know there are people who don't agree with doing this (indeed it isn't done in Icicle or AMP).

My personal vote is consistency with existing PSRs.

In Amp we currently have two watcher options:

• keep_alive
  Our event loop will exit once no more watchers are active. If keep_alive is set to false, the watcher won't count against the "active watcher count".
• enable
  Watchers are enabled by default when created. This options allows to create disabled watchers, e.g. write watchers that don't need to be enabled yet.

Do we want to integrate both like it's done in Amp via an $options array?