Review

[kelunik]

Could you please all review the full specification including all documents and documentation comments and post a comment afterwards here? Please review carefully and don't just put your thumb up here.

@trowski @assertchris @AndrewCarterUK @bwoebi @WyriHaximus @rdlowrey @sagebind @jsor

[joshdifabio]

Apologies for bringing this up again, as I think we discussed it previously but I can't find where, but what does the following mean in Loop?

An unique identifier that can be used to cancel, enable or disable the watcher.

Unique per-driver instance or per-process?

[kelunik]

Unique to the driver, but I guess we need to add that to the docblock.

[joshdifabio]

Unique to the driver, but I guess we need to add that to the docblock.

Okay. Now that Driver is an abstract class, and not an interface, I think it would be trivial to enable watcher ID's to be unique per-process and to detect when a watcher ID has been passed to the wrong driver instance, so I'd like to open a PR for this and get some feedback from loop implementors. I'll do so this afternoon or this evening.

[bwoebi]

@joshdifabio basically:

public static function getNewIdentifier() {
    static $i = "a";
    return $i++;
}

I do not object per se, but perhaps for some reason implementations may want to use resource ids or object hashes as watcher id … It'd save them an extra mapping.

Thus there may be some advantage in giving implementations freedom over their ids.

[joshdifabio]

Thanks @bwoebi. My idea is similar but includes a driver identifier to highlight situations where the application is attempting to reference a watcher from a different driver instance.

I do not object per se, but perhaps for some reason implementations may want to use resource ids or object hashes as watcher id … It'd save them an extra mapping.

If watcher ID's must be unique per-driver instance then I guess that rules out object hashes?

Thus there may be some advantage in giving implementations freedom over their ids.

Sure, I accept that this might be the case. I'll make a quick PR this evening and then perhaps you guys who are responsible for implementing loops can comment. I'm open to the possibility that it might be the wrong approach.

[bwoebi]

If watcher ID's must be unique per-driver instance then I guess that rules out object hashes?

Ah right.

@joshdifabio Please PR it then, I'll probably approve it.

[joshdifabio]

Cheers. I'll do so after work this evening.

[drealecs]

Hi guys,

I'm also trying to review the code and documentation.

So far I've analyze the code and I'm going to look for implementations and usage examples to be sure I understand the intent.

My first question is related to the usability of the loop registry.
Probably I didn't really understood the default DNS resolvers use case.
Can we provide an example for it in example/documentation?

[kelunik]

@bwoebi wanted to provide a PR explaining all the things in a META document.

@drealecs You can see an actual use case here: https://github.com/amphp/dns/blob/badf3a91003814ce574030a3d695978099d15301/lib/functions.php#L16-L25

Basically everything that's static and needs the loop should use something stored to the loop state.

[drealecs]

Thank you, I understood why it is needed.
The use case is for a component that uses the loop (by setting a callback bound to an instance in the loop) and it needs to keep a registry map of loop to instance in order for a client to retrieve the specific instance of the component bound to the current loop.
And we helped with this by implementing the registry in the loop.

I believe there might be other ways to do this, not necessary having a registry implemented in the loop.
I have an idea but let's get back to this in a separate discussion, a bit later, after I'll give it time and eventually prepare a PR.

[bwoebi]

Note that in case we shift that responsibility onto the respective classes (e.g. the DNS drivers), it does not know when a loop has been freed and will reference the state forever, even though the particular loop instance has been long abandoned. … That's what one calls a memory leak and must be avoided.
The registry serves that exact need, by removing the references to the now obsolete objects alongside with the loop.

[drealecs]

Indeed, I thought about it more and seems simpler and safer this way.
My idea was about the ability to check a watcherId for the current loop. Avoiding memory leaks would have been achieved by the loop notifying about the fact that it stooped using the callback.

[drealecs]

As a different topic, I saw some discussion about how a loop should work.
I understand there are some disagreements related to how far should this project go into the pure/correct way and how much it should stay focused on interoperability.

As I'm not actively using an async library at this time, I tend to care more about what form should be used to better cover all aspects.

I was thinking that you could even have something like this in 'index.php'

Loop::execute(function(){require('index-async.php');});

And with this, I got to an idea that maybe some of you have already though about it and can come up with an answer why it would work or not:

Add event loop in the PHP runtime directly, as a PHP extension
When the PHP process starts, just before executing the first file, it will start a loop and execute the file while the loop is running... similar with how a deferred execution works.

Namespaced functions for adding watchers and controlling them will be available.
Also there is a need for some functions to allow wiring in the loop a socket event handler, signal event handler if none is provided.

I don't have a very in detail knowledge about what a PHP extension can do but having something like that would be a lot better for usability and future interoperability.

[kelunik]

Yes, we already thought about that, but not in public yet. But it'd probably be much more like https://github.com/php-ds with this package here being the polyfill and the extension replacing the polyfill if available.

The issue with your approach is that it requires the extension then, it's not polyfillable in userland. And if there's only one truly global loop, how do we test then? Require everything in every test to clean up each and every watcher? Test with a new process for each test case?

The main point for having an extension would be the possibility of writing other extensions directly integrating with the loop.

[drealecs]

I wasn't thinking that we need to be able to provide a polyfill for the extension; maybe there is no need to.
If a library want to use the event loop, it should require "ext-eventloop:*".

Maybe a way would be to have this in the beggining of a file:

<?php

require_once 'vendor/autoload.php';

if (!\EventLoop\isRunning()) {
    return \EventLoop\runStartingWithFile(__FILE__);
}

//... continue with the execution

Related to testing, I would ask "how do you test an async javascript functionality?"
Searching for an answer would point me to this: https://www.martinfowler.com/articles/asyncJS.html
I don't believe we really need to clean up the loop. Each tests should take care of this.

[kelunik]

I wasn't thinking that we need to be able to provide a polyfill for the extension; maybe there is no need to.
If a library want to use the event loop, it should require "ext-eventloop:*".

Requiring an extension to be installed greatly hinders adoption.

If you read https://www.martinfowler.com/articles/asyncJS.html, you'll see that it mostly just advises not to test async code, but mock all async components in a sync way. https://www.martinfowler.com/articles/nonDeterminism.html is the more important one, talking about isolation of test cases. It's not a good idea to require test cases to clean everything up. All other tests shouldn't be affected either way. Loop::execute ensures there's a clear scope and every test case has its own scope.

You might also want to have a look at https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md, which uses one process per test, like PHPT tests do. Generally, you want to have a look at how Node tests are written, not client side JS tests, because client side JS really usually can avoid async APIs.

Now you might say we can also use PHPUnit's setUp to replace the loop with a new one / reset the current loop, but it's easy to forget and you might end up with non-deterministic, non-isolated tests affecting each other without realizing it's due to the missing Loop::set(Loop::createDriver()); / Loop::reset().