Remove requirement to suppress warning #125
Conversation
In order to not waste resources on suppressing an E_WARNING and to accommodate debuggers that may ignore the suppression, we can use `stream_select` to check to see if there is a read available before we attempt to accept the socket. From the php docs on the read parameter: `The sockets listed in the read array will be watched to see if characters become available for reading (more precisely, to see if a read will not block - in particular, a socket resource is also ready on end-of-file, in which case a socket_read() will return a zero length string).` The warning about using `===` for comparisons is when checking for an error, in this case, we really only want to continue if we have a truthy value. The function returns `FALSE` for errors OR the (int) `number of socket resources contained in the modified arrays`.
|
Fixes #124 |
|
Loop::onReadable does basically that, so if the stream isn't readable, the watcher shouldn't be invoked. |
I agree with you, but when used inside this |
| $empty = []; | ||
| // To prevent debuggers from triggering when stream_socket_accept() emits E_WARNING on client accept failure, | ||
| // first check to see if there is a stream waiting for us. | ||
| while (stream_select($read, $empty, $empty, 0) && $client = \stream_socket_accept($server, 0)) { // Timeout of 0 to be non-blocking. |
There was a problem hiding this comment.
As this is a hard dependency on stream_select, it will usually break with file descriptors > 1024, which is the default limit for stream_select. I think a custom error handler might be better if we want to avoid the warning.
|
The obvious solution here is to not use a while loop to accept clients but leave the watcher active and rely in the event loop to invoke the watcher again. However, we still may need to retain the error suppression. We use error suppression when accepting a single client in the socket library: https://github.com/amphp/socket/blob/23036caf1a8a236e908c3289222222a35ded53cf/src/Server.php#L88 I assume this is for a reason? Maybe accepting fails occasionally even when the watcher is activated? |
|
I'm sorry I can't add too much more to this conversation, this is a bit beyond the scope of my knowledge. |
|
We still need the suppression, because of multi-process scenarios, where multiple processes listen on the same socket, indeed. Not sure whether we need the |
|
Ah yes, I knew there was a good reason. I guess our own error handler is the correct fix (and probably should be used everywhere we were forced to use |
|
We've switched out uses of For AMPHP v2, this is far from the only usage of |
In order to not waste resources on suppressing an
E_WARNINGand to accommodate debuggers that may ignore the suppression, we can usestream_selectto check to see if there is a read available before we attempt to accept the socket.socket_select ( array &$read , array &$write , array &$except , int $tv_sec [, int $tv_usec = 0 ] ) : intFrom the php docs on the
&$readparameter:The warning about using
===for comparisons is when checking for an error, in this case, we really only want to continue if we have a truthy value. The function returnsFALSEfor errors OR the (int) number of socket resources contained in the modified arrays.Tested on php7.4/Linux