Enforce unique-per-process watcher identifiers

[joshdifabio]

This PR aims to enforce unique-per-process watcher identifiers. It does so in a relatively cheap way, adding a single function call per watcher created, and also provides a more expensive validation method which drivers should only call when they do not recognise a watcher identifier. This validation method is able to distinguish between the following error cases:

• ~Watcher identifier invalid~
• Watcher identifier passed to wrong driver instance
• ~Watcher identifier no longer valid~

Edit: The PR is now more efficient but provides less fine-grained errors.

[joshdifabio]

What do people think about adding a WatcherIdentifierExpired exception class (there might be a better word than expired) to distinguish between truly invalid watcher identifiers and ones which have simply already expired?

[joshdifabio]

Unless I'm missing something, there isn't a significantly more efficient way of checking the format of a watcher than preg_match(), but I think the only important thing is erroring when a watcher identifier is passed to the wrong driver instance. Right? I don't think validating the format of the identifier is really very useful.

[trowski]

Can we add to the spec that it is acceptable to not guarantee unique-per-process identifiers in production? I would definitely be a supporter here if loops can skip it if zend.assertions = 0.

[joshdifabio]

Can we add to the spec that it is acceptable to not guarantee unique-per-process identifiers in production?

What's the reasoning? Have you checked the latest version of the changes?

[rdlowrey]

What if the spec instead allows loops to receive a callable (or IdFactory) responsible for generating watcher IDs? In that way you avoid the fcall overhead for folks who don't need it at the minimal cost of an additional if check (because increment operators could be used if the factory wasn't set).

I'm concerned about the performance implications of adding a regex check on every watcher creation. It would seriously inhibit loop performance ...

[bwoebi]

I think I can live with the current version of the PR.

[joshdifabio]

I'm concerned about the performance implications of adding a regex check on every watcher creation. It would seriously inhibit loop performance ...

There isn't a regex check on every watcher creation. Previously there was a regex check for unrecognised watcher identifiers but I've removed that. Please take a look at the latest PR and see if you still think the functionality needs to be opt-in.

We can't use a bool type declaration.

Well spotted, thanks.

[joshdifabio]

I'll squash the commits if we decide to merge this.

[joshdifabio]

@rdlowrey @trowski The overhead here is a single function call when creating a watcher, and two more (a method which in turn calls strpos()) when a loop driver encounters an unrecognised watcher. The benefit is that we can guarantee that clients shall never unknowingly enable/disable/cancel watchers from the wrong driver instance. Looking at the implementations of Amp's loops, for example, this overhead really seems very minimal. Do you still think this should be opt-in?

[kelunik]

I'm not sure about the must throw boolean option. I think drivers should just call the method and throw afterwards if they're in enable or so.

[joshdifabio]

I'm not sure about the must throw boolean option. I think drivers should just call the method and throw afterwards if they're in enable or so.

Yes, I think I agree.

[bwoebi]

@joshdifabio Can you please look at @kelunik comments?

[joshdifabio]

I'll aim to rebase and apply Niklas's recommendations this evening.

[kelunik]

@joshdifabio ping.

[kelunik]

Closing, as project discontinued for now.

[kelunik]

Just for the record: I'm currently facing an issue where a __destruct cancels a watcher of another loop.

[joshdifabio]

Just for the record: I'm currently facing an issue where a __destruct cancels a watcher of another loop.

Scary. Make it impossible.