Php-amqplib: Handling of SOCKET_EAGAIN in StreamIO not working in PHP 7.4

[Majkl578]

Version affected: 2.11.0

Description
PHP 7.4 contains a slight BC break regarding fwrite() - it now returns false instead of 0 on certain errors. This was done in this commit: https://github.com/php/php-src/commit/d59aac58b3e7da7ad01a194fe9840d89725ea229#diff-5b57169b62cfa786ff579c633a24d92cR1221-R1223
To explain what happens I'll quote the commit message:

The php_stream_read() and php_stream_write() functions now return
an ssize_t value, with negative results indicating failure. Functions
like fread() and fwrite() will return false in that case.

As a special case, EWOULDBLOCK and EAGAIN on non-blocking streams
should not be regarded as error conditions, and be reported as
successful zero-length reads/writes instead. The handling of EINTR
remains unclear and is internally inconsistent (e.g. some code-paths
will automatically retry on EINTR, while some won't).

I'm landing this now to make sure the stream wrapper ops API changes
make it into 7.4 -- however, if the user-facing changes turn out to
be problematic we have the option of clamping negative returns to
zero in php_stream_read() and php_stream_write() to restore the
old behavior in a relatively non-intrusive manner.

How to reproduce

When fwrite() in StreamIO::write() returns false on some recoverable error, e.g.: _Notice: fwrite(): send of 69 bytes failed with errno=11 Resource temporarily unavailable_, AmqpLib throws AMQPRuntimeException('Error sending data') in PHP 7.4 instead of retrying. I don't have exact repro code on hand at the moment.

Possible Solution
The check for $result === false will need to be adjusted to correctly handle _expected_ cases like SOCKET_EAGAIN.

Additional context

This only affects PHP 7.4+

[Majkl578]

Here's temporary workaround that makes it work again:

--- PhpAmqpLib/Wire/IO/StreamIO.php.orig    2019-12-18 13:19:19.000000000 +0100
+++ PhpAmqpLib/Wire/IO/StreamIO.php 2019-12-18 13:20:48.393231153 +0100
@@ -20,6 +20,9 @@
     /** @var resource */
     private $sock;

+    /** @var bool */
+    private $writeErrorRecoverable = false;
+
     /**
      * @param string $host
      * @param int $port
@@ -260,7 +263,7 @@
                 }
             }

-            if ($result === false) {
+            if ($result === false && !$this->writeErrorRecoverable) {
                 throw new AMQPRuntimeException('Error sending data');
             }

@@ -269,7 +272,7 @@
             }

             $now = microtime(true);
-            if ($result > 0) {
+            if ($result !== false && $result > 0) {
                 $this->last_write = $write_start = $now;
                 $written += $result;
             } else {
@@ -299,9 +302,12 @@
             case $constants->SOCKET_EWOULDBLOCK:
             // stream_select warning that it has been interrupted by a signal - EINTR
             case $constants->SOCKET_EINTR:
+                $this->writeErrorRecoverable = true;
                 return;
         }

+        $this->writeErrorRecoverable = false;
+
         parent::error_handler($code > 0 ? $code : $errno, $errstr, $errfile, $errline, $errcontext);
     }

I don't think it's elegant enough to be worth a PR but let me know if I am wrong. :smiley:

[ramunasd]

Thanks for reporting. ATM we do not support PHP 7.4 as other versions.
However any warning from fwrite() is handled by custom error handler, which tries to extract error code and behave differently.
Please take a look at:
https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/IO/StreamIO.php#L294

[ramunasd]

@Majkl578 please correct me if i'm wrong, but since PHP7.4 fwrite() returns 0 [1][2] on SOCKET_EWOULDBLOCK and SOCKET_EAGAIN. We throw AMQPRuntimeException('Error sending data') only when return value is strictly equal to false.

From BC documentation[3] fread() and fwrite() will now return FALSE if the operation failed. Previously an empty string or 0 was returned. EAGAIN/EWOULDBLOCK are not considered failures..

[1] https://github.com/php/php-src/blob/php-7.4.0/ext/standard/tests/streams/eagain_is_not_an_error.phpt
[2] https://github.com/php/php-src/blob/php-7.4.0/main/streams/plain_wrapper.c#L345
[3] https://www.php.net/manual/en/migration74.incompatible.php#migration74.incompatible.core.fread-fwrite

[Majkl578]

@ramunasd I believe that commit message is wrong, it returns 0 only in case of fread(), not fwrite() (in our case).
I spent half a day debugging this issue and it was caused by false return on Notice: fwrite(): send of 69 bytes failed with errno=11 Resource temporarily unavailable.

Maybe it's a bug in PHP and fwrite() was unintentionally not handled properly. It may be related to only some types of streams... I guess the plain wrapper you referenced is for STDIO, not for network streams.
But implementation of streams in PHP is a bit hard to understand. @nikic can you please help us understand what's going on and whether it's a bug, feature or me being wrong?

[nikic]

Are you using non-blocking socket streams?

[Majkl578]

@nikic Yes, it's enabled during connect here (and I checked it's called): https://github.com/php-amqplib/php-amqplib/blob/v2.11.0/PhpAmqpLib/Wire/IO/StreamIO.php#L137-L143

[nikic]

Reported: https://bugs.php.net/bug.php?id=79000

[Majkl578]

Thank you @nikic and sorry for bothering you. :)