Enforce unique-per-process watcher identifiers

[joshdifabio]

As discussed briefly in #126, this PR proposes enforcing that watcher identifiers are unique-per-process, instead of just unique-per-driver instance. I believe that this will eliminate the potential for some scary and confusing bugs where multiple driver instances are used within a single process and watcher identifiers are unknowingly, possibly silently, passed to the wrong drivers.

I took an approach which allows loop implementations to define their own "local" watcher identifiers, which must be prepared using the provided method getGlobalWatcherId() before being returned to clients. If it isn't really necessary for loop implementations to define their own watcher identifiers then we could potentially simplify further by generating the watcher identifiers incrementally in the abstract Driver class. This would have the added benefit of allowing us to distinguish between already removed watcher identifiers and invalid (never returned by the driver) identifiers.

If some of you loop implementors could give feedback that would be appreciated; obviously you're more qualified than I am to know whether there are problems with this approach.

@bwoebi

Edit: I'll add tests if people like the feature.

[trowski]

I think that this approach adds too much overhead and complicates what should be very simple. If we wish to provide a mechanism for watcher identifier generation, the following is all that's necessary in Driver:

private static $nextId = 'a';

protected static function createWatcherIdentifier(): string {
    return self::$nextId++;
}

Watcher manipulations are frequent and fundamental operations in the loop. I do not want to overburden them with many function calls and string manipulation to catch what should be a rarity - passing a watcher generated in one driver instance to another driver instance.

In general I do not think that enforcement of process-unique watcher identifiers is necessary. However, if we do the only approach I would support is a simple one like above.

[joshdifabio]

@trowski thanks for taking a look.

Regarding this being an edge case; I think we have to assume that if it can happen, it will happen. Loops are completely fundamental to applications which use them; I don't think we should be prescribing any level of risk to applications which use the standard if we can avoid doing so.

Regarding your proposal: You are suggesting that drivers don't need to be able to define their own watcher identifiers. If that is the case then we can simplify a lot:

    private static $nextDriverId = 'a';
    private $driverId;
    private $nextWatcherId = 'a';

    final protected function createWatcherIdentifier()
    {
        if (!isset($this->driverId)) {
            $this->driverId = self::$nextDriverId++;
        }

        return "{$this->driverId}-" . $this->nextWatcherId++;
    }

    /**
     * This method would only be called in situations where a driver doesn't
     * recognise a watcher ID in order to differentiate between wrong driver,
     * already terminated watcher and a watcher which was never created
     */
    final protected function validateWatcherIdentifier($watcherId)
    {

    }

This would mean that calls to cancel() on the wrong driver instance would error, whereas it wouldn't be possible to detect these cases with your proposal. This would allow us to eliminate the possibility that watchers remain active when the client thinks they've been cancelled, or alternatively that a client disables the wrong watcher.

I don't believe this approach would add any significant overhead. Bear in mind that implementations will already have to call some functions each time they receive a watcher identifier in order to validate that it's a string, since we aren't using PHP7 & therefore don't have access to scalar type hints.

[kelunik]

@joshdifabio I think I like the last suggestion. It doesn't add much overhead, but ensures watchers are passed to the right instance. Running multiple loops isn't common, that means running them is extra risky, as people are not used to it.

[joshdifabio]

Superseded by #137.