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.