Operations on Invalid Watchers Must Throw

[bwoebi]

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?

[kelunik]

So you want to throw an exception then? We should then also throw exceptions if unreference etc. are called with an non-existent watcher.

[bwoebi]

For non-existent watchers, sure. Passing these is a mistake which may in edge cases even alter the behavior of the program when the watcher id gets reused.

[kelunik]

If you think it's a duplicate of #61, then I'll give this one a better title.

[bwoebi]

PR is at #63.

[joshdifabio]

Is there any reason cancel() can't be idempotent? (I.e. perform a null-op when watcherId doesn't exist instead of throwing.) I can't think of any reason I'd want it to throw in such cases.

[bwoebi]

as watcher ids may be reused

This is the main point. If a watcher id gets reused, you may end up with actually cancelling another watcher than you intended. Thus doubly cancelling or operations on invalid watchers must be a programmer error. (which typically happens spuriously, i.e. if you use spl_object_hash() internally as id; thus very bad.)

[joshdifabio]

Is this definitely preferable to requiring that watcher ID's be unique for the duration of the process?

To allow unique watcher ID's across all drivers, the drivers could return watcher ID's which are unique per-driver instance and Loop could prepend those ID's with a driver ID (something to uniquely identify a driver instance). This would ensure that the watcher ID's returned by Loop were unique for the duration of the process. It could also mean we don't have really long strings (from spl_object_hash) being stored all over the place, although I don't know how important that actually is.

Edit: I'm assuming that drivers could return watcher ID's which are unique per-driver instance would be fairly straightforward if a counter was used.

[bwoebi]

Actually, I think I'm possibly wrong here...

just use a static $watcher = "a"; which you can increment on every assign … is unique and the strings are short enough …
After a hundred trillion of watchers you still only have 10 chars … and 10 chars will probably cover the life-time of a server… (i.e. 11 chars are enough for 10 million watchers per second for 11 years.)

And as we don't need unpredictability of watcher ids here, this should be fine …

So, let me revert this commit and redecide… I've merged too quickly, indeed.

[joshdifabio]

Good point, a single static counter would be a much more elegant implementation. Could even have a new method on Loop:

public static function nextWatcherId() : string;

[bwoebi]

Do we need to have a global counter? A counter per driver should be good enough?

[joshdifabio]

I was assuming we'd want to guarantee that the methods on Loop return a unique (for the duration of the process) watcher ID. Unless we do something like I suggested in my earlier comment where we concatenate a driver instance ID and a driver watcher ID, but I figured it'd be more elegant (and efficient?) to have a single global counter (which is actually what I thought you were suggesting).

[bwoebi]

That way we'd be introducing a dependency on the Loop class from within the driver implementations; which both should be rather decoupled...

[joshdifabio]

That's true, although the driver implementations will be in packages which depend on this package anyway, so I don't know how much difference that would really make. Applying GRASP (high cohesion) would seem to suggest that such a method might belong in the Loop class at least.

Just so I understand, are you thinking that watcher ID's returned by Loop should be unique for the duration of the process, or just unique to the current driver?

[bwoebi]

I'm not sure about unique for a process.
But they definitely must then be unique per driver.

Perhaps we want to have a tiny function (which then would be unique per process):

function nextWatcherId() {
    static $id = "a";
    return $id++;
}

But independent of the loop, just a standalone function whose output is guaranteed to not have appeared yet.

[joshdifabio]

Personally, I think that would be a good solution. Guaranteeing uniqueness for the duration of the process is IMO good to do given that it's so easy.

[trowski]

I think a unique watcher ID per driver would be sufficient, though as @bwoebi pointed out, a function would work, but this is something that Loop should not provide in my opinion.

Using an invalid watcher ID generally represents a programming error, so I think throwing makes a lot of sense when an operation is attempted with an invalid ID.

[bwoebi]

Double cancel() may though not be a programming error, just like double Loop::stop() may not be. (e.g. whatever Awaitable resolves first, will cancel first, the second Awaitable will also cancel. This prevents the need of a shared variable to mark the watcher as already cancelled.)

Using an invalid watcher id (i.e. one which may never be valid until now) MUST throw.

[bwoebi]

@trowski The only thing I may agree that we throw here, is enable() on a cancel()'led watcher.
This one actually has influence on whether the program continues to work or not (example: call enable on cancelled watcher, return an Awaitable, which then never gets resolved as the watcher is never called)

But disable() on a cancel()'led watcher is IMO equivalent to disable()'ing an already disable()'d watcher. These shall be idempotent.
At least as long as there is no particular technical reason against that, I don't see why it should throw.