Streams closed before watcher is enabled

[ghost]

Since read / write watchers are enabled after the current tick of the event loop there is a possibility that a stream resource is closed by another watcher within the same tick. This causes trouble with many loop backends (ev, event, uv) because they fail at creating new event watchers when the stream resource has been closed.

This is an edge case but i think that the specification should be updated to define expected behavior in this case. It would be sufficient to have loop implementations check for valid resources as new stream watchers are enabled for the first time and call the watcher callback once if a closed / invalid resource is detected.

[bwoebi]

Hmm, good point. That scenario never occurred to me, but creating an event watcher on an invalid resource obviously doesn't work. There's also the scenario where a resource gets closed after the watcher had been registered.

So, we can generalize this to: what should happen if a closed socket lands in the loop.

We need a general behavior what should happen in that case - I'm slightly in favor of just throwing an exception whenever that happens. — The main issue is there that we'd need to check each resource before each call to stream_select() or the loop extensions native function.

So … uh, don't know - it's not like there would be any callback for userland upon resource invalidation or such…

[ghost]

Resources can be closed after watchers have been created, however it does not fail with an error / exception in any loop backend i tested so far (ev, event and uv). All of these will recognize a closed resource and invoke the registered callback in the same way as they would on EOF. Therefore checking for a valid resource is only needed when creating the stream watcher / event for the first time.

Checking for valid resources is not required when enabling a watcher based on stream_select() because it will be reported as readable / writable next time the function is called. This automatically leads to the watcher callback being called anyways if you do not check for a valid resource every time a stream watcher triggers.

Checking for valid resources during creation of new (ev, event, uv)-based watchers is important because it can break the backend (uv likes to segfault in this case).

I think the most efficient and consistent way to deal with the situation is to call the watcher callback because it will happen anyways if the stream is closed immediately after the watcher was enabled. You can have a look into this behavior in the loop implementations of koolkode/async. This solution worked out very well for me so far...

[bwoebi]

Well, just to be sure - we are talking about manual fclose() calls or similar while there's a write/read watcher installed?

It just is reported as writable/readable if the resource has been closed by the remote endpoint, but not when we close it ourselves with fclose() - in this case the loop is just running indefinitely. E.g. stream_select() will just ignore these resources.

[ghost]

I did some testing and yes you are right about the stream_select() / local close problem. The resource existed in the read / write array because it was the only resource being watched and stream_select() failed due to no valid read / write arrays being passed. That basically leaves me with two options: check all resources before calling stream_select() or diff read / write array after stream_select() and check for valid resources...

I will check the other loop backends tomorrow, it will not be easy to deal with this in a consistent and performant way...

[bwoebi]

I've just checked libuv - it behaves the same way.

Either we declare it undefined behavior, or we'll end up with a significant performance hit. Implementations shall be free to warn the dev if they notice this happening (e.g. in a debug mode), but it doesn't sound very appropriate to force implementations to impose this performance penalty on the user.

[ghost]

I checked libev, it triggers a warning next time the loop is run.

Warning: EvLoop::run(): Libev error(9): Bad file descriptor

Seemingly not possible to avoid this without adding lots of very slow resource checks. I agree with this being undefined behavior. Maybe the docs should state that locally closing a watched resource will cause problems.

[kelunik]

Which forms of notification should be allowed?

[bwoebi]

@kelunik Does not need to be specified as notifying itself isn't required. It may be a custom callback, it may be an Error thrown, or just an E_WARNING issued by the event loop extension itself… whatever.

[kelunik]

@bwoebi If it's something that throws, then it should be defined where it should be thrown IMO. Otherwise it's not required to be specified.