Logic error in Socket::getType() results in "Don't know how to handle a response of type" exception

[dominics]

The method Socket::getType() (https://github.com/mariano/disque-php/blob/39bc697/src/Connection/Socket.php#L192L207) is notated via PHPDoc as @return string A single char - that's not the case; it's actually ?string due to a small logic bug.

The problem is: if $this->socket is at EOF, the initial while condition is already false, and fgetc is never called. This makes the value of $type strictly equal to null, not false or the empty string.

From there, the check at the end of the method ($type === false || $type === '') will fail to match, and the correct exception is not thrown (should probably be "Nothing received while reading from client" - this usually happens during timeout).

This results in the user getting a ResponseException (when there was no response!) instead of a ConnectionException. The message is confusing too: "Don't know how to handle a response of type".

[dominics]

We should also look at the wisdom of calling feof() on a socket in any case. Why are we doing that?

• From what I've seen/read, feof() can return true when PHP will still return data from fgets() (because it's in internal buffers/$metadata['unread_bytes']).
• And isn't feof() on a socket blocking if the connection isn't already closed by the server? (not in PHP apparently? Or at least, only when the resource was opened by fsockopen? Manual still has a warning.)
• And this is testing the EOF indicator, not the socket itself (so some other IO operation must have already been interrupted or the indicator wouldn't be set)?

Is there somewhere this code was copied from? It doesn't seem to be handling sockets correctly.

(Also c.f. the Predis implementation which doesn't use feof here: https://github.com/nrk/predis/blob/v1.1/src/Connection/StreamConnection.php)

Edit: Hmm, regarding Predis' use of feof, see predis/predis#164 - connection timeouts on the underlying socket (e.g. via socket_default_timeout) may be an unsolved issue without feof - so, perhaps fine. I hereby withdraw this part of the issue until I have better evidence. :) - at least they confirm reports of issues when relying on foef

Edit: I found https://stackoverflow.com/a/26557243/10831 quite useful for understanding what feof() is actually checking, and why while (!feof(fp)) is almost always wrong (and why we should just be checking the fgetc result instead).