search

async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
169 stars 9 forks source link

Exception to be thrown for invalid streams #92

Closed kelunik closed 8 years ago

kelunik commented 8 years ago

We cannot declare a type for the $stream parameter in onReadable and onWritable. Which exception should be thrown if there's an invalid argument passed? Should we add a new InvalidStreamException? Maybe an own (more generic) InvalidArgumentException? Should we use the one of Spl instead?

kelunik commented 8 years ago

Same case for invalid signal numbers.

bwoebi commented 8 years ago

Do we actually need to standardize that? If you do weird things which are not intended, can't we just have implementation defined behavior?

I do not see a need for interoperability for invalid operations stemming form an obvious programmer error.

You'd never want to catch such Exceptions [i.e. except in generic catch-all handlers].

If implementations want to provide helpful error messages, that's at their discretion, but standardizing it holds no value. The point of the standard is having valid operations and Runtime exceptions interoperable.

kelunik commented 8 years ago

If you do weird things which are not intended, can't we just have implementation defined behavior?

We could, but I see no reason to leave it open.

I do not see a need for interoperability for invalid operations stemming form an obvious programmer error.

The most important point here is simply consistency, no matter which implementation. Currently there's nothing defined. Should invalid streams just be ignored, as they're just never readable? Should there be an exception? ...

bwoebi commented 8 years ago

I see no reason in standardizing that. I'm sure there's a ton of ways you can corrupt each implementation to yield different results, regardless of what you do.

I even wouldn't define it as e.g. you can't really even check ahead of time whether the stream is valid to be passed to the different extension backends. There may be streams you cannot pass to the backend, but still have a fd (for example the kqueue file descriptor, which you can explicitly access with fopen("php://fd/5", "w+") (whatever the fd is)). Let's not go down the rabbit hole of whitelisting allowed stream types here.

Thus, the behavior shall be implementation defined.

Re invalid signal numbers: maybe your system has custom signal numbers outside of the typical range (i.e. 0-31) and the application is just targeting that particular system? If an implementation is able to reject it - fine. For example with pcntl you have no choice except trying to actually register it; if it fails, it will fatal error.

Again: implementation defined is best.

kelunik commented 8 years ago

@bwoebi In that case we should add something like @throws Throwable|Exception Implementations might throw if they can't register a write watcher for the given stream.

bwoebi commented 8 years ago

It should just specify: Behavior when implementations cannot register a write watcher for a given stream is not defined by this interface.

Basically, implementations may throw, they may warn, they may fatal. Or just be quiet about it there and explode somewhere else.

Your @throws tag is somewhat assuming that implementations will be able to detect failures upon call of onWritable(), which may not be given.